With PHP 5.5 I get the following warning when submitting a form with filefield_path enabled file fields:
Deprecated function: preg_replace(): The /e modifier is deprecated, use preg_replace_callback instead in _filefield_paths_replace_path() (Line 328 in /var/www/xxx/sites/all/modules/filefield_paths/filefield_paths.module).
| Comment | File | Size | Author |
|---|---|---|---|
| #32 | deprecated_e-2103151-32.patch | 2.57 KB | kolier |
| #26 | deprecated_e-2103151-26.patch | 2.19 KB | sumeet.pareek |
| #14 | comment-13-regex-groups.png | 32.84 KB | neRok |
Comments
Comment #1
blitux commentedThe /e modifier in preg_replace is deprecated on PHP 5.5 because it can be used to perform arbitrary code execution, as stated in POSIX Pattern Modifiers Docs.
Comment #2
graceangell@gmail.com commentedWhy this bug is "Unassigned"?, Сan we expect to see the fix in the next version?
Comment #3
david_garcia commentedComment #4
j0rd commentedI think the code in the patch above will require a high version of PHP to work. I recommend simply making a normal function and calling it to reduce PHP version requirements.
Comment #5
huma2000 commentedjOrd: Can you provide me a patch to do it or guide me to some documentation to create one?
O'm getting the same error :(
Comment #6
david_garcia commentedMinimum php version required is 5.3 for the patch.
Comment #7
j0rd commentedMinimum version for PHP + Drupal is 5.2.5. I personally don't use that version, but I know from looking at the code, it could be re-written to work in 5.2.5.
I believe simply pulling out that anonymous function, into a real function should meet that requirement.
No reason to fix a warning in 5.5, which breaks PHP 5.2.5 - 5.3.
That's my two cents.
Comment #8
j0rd commentedComment #9
Dima_N commentedThis patch should work on all 5.x versions. Need to test.
Comment #10
kitikonti commentedi have created a patch with file with the changes from #9 for the latest dev. dont tested it yet.
Comment #11
neRok commentedClosed #2214337: PHP 5.5 The /e modifier is deprecated in preg_replace as duplicate of this issue.
Comment #12
decipheredWhile I'm inclined to agree with j0rd on both his points (public function and version issue), it can't actually be a public function because
preg_replace_callbackhas no way to passing additional data for the replacement (such as the new URL of the processed file).The patch from #9/#10 doesn't work at all, as it appears to be returning an array when a string is expected, but the
create_functionfunction does look promising.More work to be done here.
Edit: Since I wrote the above I did do some more work, and it's about 90% complete, I will try to get it finished off and committed in the next 24 hours.
Comment #13
neRok commentedThis one is a bit of a headache. It seems difficult to pass parameters through preg_replace_callback in PHP<5.3, and we need parameters passed in order to replace the image style tokens. If it weren't for the tokens, I dont think it needs a callback at all, just a straight up replace.
I've re-written the code without any callbacks, see the bottom. It is a bit longer now, but it works. Sorry about the lack of patch...
Things I have done;
?itok=This patch does away with
_filefield_paths_replace_image_derivative_token, it just straight up replaces the token.It also does away with
_filefield_paths_replace_path_uri_scheme. I got rid of this function as I didn't understand the need for it, as FFP cannot effect the 'scheme'. It could be implemented though.The only 'bad' thing about my code is if the same image style is linked more than once, it will request the tokens more than once, and also try replace the string more than once (even though the string would get replaced on the first match). It doesn't cause any problems/errors, but it is happening.
Comment #14
neRok commentedThe attached image outlines the regex results/groups for the code in comment-13. The sample text has 2 thumbnails (absolute and relative urls) and the full size image, to cover all bases.
The regex groups are basically the same for the modules current regex, except group 6 doesn't exist (and 7 is 6, and 8 is 7).
EDIT: just realised whilst uploading this image, my code wont capture styles without tokens (ie older version of drupal 7). It probably needs to be...
Or something similar. I haven't got time to test, it's home time!
Comment #15
Dima_N commentedDeciphered
preg_replace and preg_replace_callback returns equivalent result. Both functions returns an array if the subject parameter is an array, or a string otherwise.
In my case preg_replace_callback also accepts a string returned by create_function.
Comment #16
decipheredDima_N, I assure you that the patch from #9/#10 did not for me, when I tested it multiple times it returned incorrect results. If you're saying that it worked for you, then so be it, I was unable to replicate a successful result as it returned an array instead of a correct string.
I have however resolved the issue in the attached patch. If someone can confirm that it works that would be great, if not, I will likely commit it anyway as I'm confident it is working.
Comment #17
neRok commentedI tested it, and it works.
I looked at http://au1.php.net/create_function, which says
I thought I would test the performance. I put a
timer_startas the first line of_filefield_paths_replace_path, and atimer_readat the end. I changed the token and 'actively updated' a node twice, getting 1.7 and 1.84. Using the same method but with my code above, I got 0.49 and 0.46, so consistently in excess of 3x as fast.Comment #18
david_garcia commentedThat's a biiig difference.
Can this:
+ // Create an anonymous function for the replacement via preg_replace_callback.
+ $replacement_callback = create_function('$matches', "return {$replacement};");
just be moved into a regular function?
Comment #19
deciphereddavid_garcia_garcia,
As mentioned in #12, no, it can't, because preg_replace_callback() only has the ability to pass the arguments of the matches, nothing more.
I'll take a look at the concept from #13, but it will need work before it's committable, as it's currently doing quite a lot of duplication.
Comment #20
david_garcia commented¿what about this strategy?
http://stackoverflow.com/questions/9550769/passing-additional-arguments-...
class MyCallback {
private $key;
function __construct($key) {
$this->key = $key;
}
public function callback($matches) {
return sprintf('%s-%s', reset($matches), $this->key);
}
}
$output = 'abca';
$pattern = '/a/';
$key = 'key';
$callback = new MyCallback($key);
$output = preg_replace_callback($pattern, array($callback, 'callback'), $output);
print $output; //prints: a-keybca-key
The thread actually proposes another more dirty solution in wich this parameters are stored as global variables.
Comment #21
decipheredIt seems overkill to have a class for such a simple little thing. A similar thought occurred to me minus the Class, which is to set the variables into a SESSION variable.... but I really don't want to go down that route unless I must.
I will investigate neRok's suggestion first, and if it can be simplified (which I have no doubt that it can) it may be the best candidate... but I really need to step through it before I can say that with certainty....
Comment #22
zombirus commented#16 works for me
Comment #23
sinn commented#16 works fine
Comment #24
daniel wentsch commentedThanks for your patch #16 @deciphered,
against which version did you build it? It fails patching for me on beta4.
Hunk #1 FAILED at 2.
1 out of 1 hunk FAILED -- saving rejects to file CHANGELOG.txt.rej
patching file filefield_paths.module
Hunk #1 FAILED at 298.
1 out of 1 hunk FAILED -- saving rejects to file filefield_paths.module.rej
Comment #25
neRok commented@Daniel Wentsch, you should patch against dev.
Comment #26
sumeet.pareek commentedHere is the patch by @Deciphered in #16 rerolled against the module version 7.x-1.0-beta4
Comment #27
interdruper commentedPatch #16/#26 works fine for me.
Comment #28
Fidelix commentedI tested it as well.
Patch applies to current stable version and fixes the issue.
Comment #29
gkelly commentedI added patch #26 to version 7.x-1.0-beta4 and the issue was resolved. Thanks.
Comment #30
daniel wentsch commentedThanks a lot!
Comment #31
rudiedirkx commentedWorks for me too. Any time on a new release? It's been 17 months since beta4.....
Comment #32
kolier commentedReroll for 7.x-1.x-dev.
Git repo: https://github.com/drupal-issue/filefield_paths/tree/2103151
Comment #33
kerios83 commented@Sumeet.Pareek #26
Thanks! I have tested it and it's working very well.
+1 for a release.
Comment #34
francescosciamanna commented#26 worked like a charm(Thx)! What about include the patch at least in the Dev. Release?
Comment #35
mlecha commentedUsing patch #26 to version 7.x-1.0-beta4. Works. Thank you.
Comment #36
dxxOk, patch applied successfully with current --dev version.
Comment #37
osmanPatch in #32 applies to 7.x-1.x-dev without any issues.
Fixes regex related warnings.
+1 RTBC
Thanks,
Comment #38
rodrigoaguileraThis module looks pretty unmantained. I think the first step should be for someone to become co-mantainer first
Comment #39
W.M. commentedPatch at #26 works perfectly on latest stable beta release. Tested under PHP 5.6.2.
Comment #40
alauddin commentedconfirm path #26 for version 7.x-1.0-beta4 works.. PHP 5.5.18
Comment #41
zanselm5 commentedWhenever I try to patch this, I get the response, "1 out of 1 hunk FAILED -- saving rejects to file filefield_paths.module.rej" ...Any support with this would be very much appreciated.
This only started happening at the turn of 2014 when I tried to upload an image on the 1st day of 2015...
Comment #42
kclarkson commented#32 applied cleanly to from the most recent git 7.x-1.x branch.
and yes this module needs some maintainer love.
Comment #43
jimsmith commented#32 worked for me as well. Thanks for the patch, @kolier.
Comment #44
nmillin commentedPatch in #32 applies to 7.x-1.x-dev without any issues.
Fixes warnings.
Comment #45
giorgosk#32 works as advertised
Comment #46
plazik commented#32 works for me too.
Comment #47
skin#32 works for me too.
Tested on version: 7.x-1.0-beta4
Comment #48
joelpittetRTBC++ bumping to major.
Comment #49
wOOge commentedConfirmed — Patch #32 works.
Comment #50
matsbla commented#26 worked for me, thanks! :)
Comment #51
mxr576I've tested the #32 first, and I've some issues with the Insert module. First time I've uploaded an image and inserted it to the content and saved the node the image wasn't show up, because the URL of the image pointed to the wrong place. I had to edit the node and delete-reinsert the image to make it work. However, it seems the #26 working with Insert.
Comment #52
mxr576(Duplicate comment)
Comment #53
askibinski commented#26 works for me (tested against beta4)
Comment #55
decipheredFixed and committed.
Comment #56
web226 commentedpatch #26 on version 7.x-1.0-beta4. Works. Thanks!
Comment #57
deciphered@web226,
There's not need to confirm the patch works anymore, it is committed and available in the current dev release, hence the issue now being marked as 'Fixed'.
Comment #59
jay.lee.bio commentedFYI, 7.x-1.x-dev also works for PHP 7.0.2. In my case, the issue also briefly scared me a bit because some fields weren't initially getting saved when creating new content, forcing me to go back and enter them again. Thank you everyone for all the hard work.
Comment #60
hubobbb commented#26 patch works for me . Thank you .
My version is:
php 5.5.25 .
version = "7.x-1.0-beta4"
core = "7.x"
project = "filefield_paths"
datestamp = "1366871711"
Comment #61
decipheredHi guys,
Please stop commenting on this issue, the issue has been fixed for months and there is no reason to use the patch anymore. If you haven't already, update to 7.x-1.0
Comment #62
kmajzlik commented