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

Merged
merged 1 commit into from Sep 23, 2013

Conversation

Projects
None yet
4 participants
@fredsa
Contributor

fredsa commented Jun 11, 2013

in make_clickable() in includes/functions_content.php
see http://www.php.net/manual/en/reference.pcre.pattern.modifiers.php

PHPBB3-11606

@bantu

This comment has been minimized.

Member

bantu commented Jun 11, 2013

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;

This comment has been minimized.

@bantu

bantu Jun 11, 2013

Member

Coding style is a bit "off" here and the following lines. See https://area51.phpbb.com/docs/31x/coding-guidelines.html

foreach($magic_url_match_args as $magic_args) {
if (preg_match($magic_args[0], $text, $matches)) {
$text = preg_replace_callback($magic_args[0], function($matches) {
global $magic_args;

This comment has been minimized.

@bantu

bantu Jun 11, 2013

Member

This should not be global but passed as an argument.

@fredsa

This comment has been minimized.

Contributor

fredsa commented Jun 12, 2013

Please take another look

@nickvergessen

This comment has been minimized.

Contributor

nickvergessen commented Jun 12, 2013

You forgot to apply the commit message things ;) like [ticket/xyz] in first line and PHPBB3-xyz in the last one.

@fredsa

This comment has been minimized.

Contributor

fredsa commented Jun 12, 2013

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?

@marc1706

This comment has been minimized.

Member

marc1706 commented Jun 12, 2013

You can actually reword your commit messages using git rebase.

@bantu

This comment has been minimized.

Member

bantu commented Jun 13, 2013

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.

@fredsa

This comment has been minimized.

Contributor

fredsa commented Jun 13, 2013

Sorry, didn't realize that I could 'git push --force' following a local
rebase/squash.

Pull request should have one clean commit now

On Wed, Jun 12, 2013 at 5:00 PM, Andreas Fischer
notifications@github.comwrote:

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.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1479#issuecomment-19363620
.

Fred Sauer
fredsa@gmail.com

}
return preg_replace($magic_url_match, $magic_url_replace, $text);
foreach($magic_url_match_args as $magic_args)

This comment has been minimized.

@bantu

bantu Jun 13, 2013

Member

Missing space after foreach, just like after if. Otherwise this looks good to me.

@fredsa

This comment has been minimized.

Contributor

fredsa commented Jun 13, 2013

Space added after 'foreach'. Let me know if any other changes are needed.

@bantu

This comment has been minimized.

Member

bantu commented Jun 14, 2013

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.

@bantu

This comment has been minimized.

Member

bantu commented Jun 14, 2013

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.

@bantu bantu merged commit 49c12ef into phpbb:develop Sep 23, 2013

1 check passed

default The Travis CI build passed
Details
@bantu

This comment has been minimized.

Member

bantu commented Sep 23, 2013

@fredsa There actually already are unit tests for this function with pretty good coverage. Sorry this took so long.

@fredsa

This comment has been minimized.

Contributor

fredsa commented Sep 23, 2013

NP. Thanks for wrapping it up.
On Sep 22, 2013 5:35 PM, "Andreas Fischer" notifications@github.com wrote:

@fredsa https://github.com/fredsa There actually already are unit tests
for this function with pretty good coverage. Sorry this took so long.


Reply to this email directly or view it on GitHubhttps://github.com//pull/1479#issuecomment-24894310
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment