Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Help Editor] Fix incorrect substr syntax #2966

Merged
merged 1 commit into from
Jul 19, 2017
Merged

[Help Editor] Fix incorrect substr syntax #2966

merged 1 commit into from
Jul 19, 2017

Conversation

johnsaigle
Copy link
Contributor

@johnsaigle johnsaigle commented Jul 19, 2017

This pull request fixes a syntax error in help_editor which caused several PHP Warnings: substr() expects parameter 2 to be integer, string given

This is the minimal change required to fix the bug. See also #2965 for a Utility method that will allow us to reuse this fix elsewhere (if needed).

@johnsaigle johnsaigle added the Category: Bug PR or issue that aims to report or fix a bug label Jul 19, 2017
@johnsaigle johnsaigle added this to the 17.1 milestone Jul 19, 2017
@ridz1208
Copy link
Collaborator

@sruthymathew123 please test

@kongtiaowang kongtiaowang added the Passed manual tests PR has been successfully tested by at least one peer label Jul 19, 2017
@driusan
Copy link
Collaborator

driusan commented Jul 19, 2017

The description in this PR is referring to itself. I think you included the wrong link.

@johnsaigle
Copy link
Contributor Author

@driusan the link is now correct

if (substr($key, "Topic") == 0) {
// Check if key starts with $needle ("Topic")
$needle = "Topic";
if (substr($key, 0, strlen($needle)) === $needle) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the logic of what this is trying to do would be a lot more intuitive if it was written as:

if (strpos($key, "Topic") === 0)

but since this fixes an immediate need I'll make that comment on the other, more long term PR and merge this..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the longer-term PR can probably be used instead of both strpos and substr if we want to go that route.

@driusan driusan merged commit 2706f1b into aces:17.1-dev Jul 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category: Bug PR or issue that aims to report or fix a bug Passed manual tests PR has been successfully tested by at least one peer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants