-
-
Notifications
You must be signed in to change notification settings - Fork 948
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
[ticket/11606] replace uses of preg_replace() /e modifier (PREG_REPLACE_EVAL) #1479
Conversation
Develop has PHP 5.3 as a requirement. This means you can not use the new array syntax. |
} | ||
|
||
return preg_replace($magic_url_match, $magic_url_replace, $text); | ||
global $magic_args; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style is a bit "off" here and the following lines. See https://area51.phpbb.com/docs/31x/coding-guidelines.html
Please take another look |
You forgot to apply the commit message things ;) like [ticket/xyz] in first line and PHPBB3-xyz in the last one. |
My original commit (cbcf31c) did contain the [ticket/11606] and PHPBB3-11606 Should I be creating a new pull request using a squashed commit history for each iteration of feedback? |
You can actually reword your commit messages using git rebase. |
You can change the branch you are suggesting for merge however you like until it is actually merged. No need to send a new pull request. Rebase, amend, etc. is possible. |
Sorry, didn't realize that I could 'git push --force' following a local Pull request should have one clean commit now On Wed, Jun 12, 2013 at 5:00 PM, Andreas Fischer
Fred Sauer |
} | ||
|
||
return preg_replace($magic_url_match, $magic_url_replace, $text); | ||
foreach($magic_url_match_args as $magic_args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing space after foreach, just like after if. Otherwise this looks good to me.
Space added after 'foreach'. Let me know if any other changes are needed. |
We should probably look into adding unit tests for the make_clickable function if there aren't any yet in order to guarantee that pre-patch and post-patch behaviour is the same and nothing breaks. |
Not sure whether the lamba/anonymous function wrapping is a good idea / is required. Maybe we should just turn make_clickable_callback() into a usable callback instead. |
@fredsa There actually already are unit tests for this function with pretty good coverage. Sorry this took so long. |
NP. Thanks for wrapping it up.
|
in make_clickable() in includes/functions_content.php
see http://www.php.net/manual/en/reference.pcre.pattern.modifiers.php
PHPBB3-11606