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).

Comments

blitux’s picture

The /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.

graceangell@gmail.com’s picture

Why this bug is "Unassigned"?, Сan we expect to see the fix in the next version?

david_garcia’s picture

Issue summary: View changes
StatusFileSize
new2.24 KB
j0rd’s picture

I 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.

huma2000’s picture

jOrd: Can you provide me a patch to do it or guide me to some documentation to create one?

O'm getting the same error :(

david_garcia’s picture

Minimum php version required is 5.3 for the patch.

j0rd’s picture

Minimum 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.

j0rd’s picture

Status: Active » Needs work
Dima_N’s picture

This patch should work on all 5.x versions. Need to test.

Index: sites/all/modules/filefield_paths/filefield_paths.module
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- sites/all/modules/filefield_paths/filefield_paths.module	(revision )
+++ sites/all/modules/filefield_paths/filefield_paths.module	(revision )
@@ -313,9 +313,9 @@
     if ($field['module'] == 'text' && isset($entity->{$field['field_name']}) && is_array($entity->{$field['field_name']})) {
       foreach ($entity->{$field['field_name']} as &$language) {
         foreach ($language as &$item) {
-          $item['value'] = preg_replace("/$regex/e", $replacement, $item['value']);
+          $item['value'] = preg_replace_callback("/$regex/", create_function('$replacement', 'return $replacement;'), $item['value']);
           if (isset($item['summary'])) {
-            $item['summary'] = preg_replace("/$regex/e", $replacement, $item['summary']);
+            $item['summary'] = preg_replace_callback("/$regex/", create_function('$replacement', 'return $replacement;'), $item['summary']);
           }
         }
       }
kitikonti’s picture

Version: 7.x-1.0-beta4 » 7.x-1.x-dev
Status: Needs work » Needs review
StatusFileSize
new1017 bytes

i have created a patch with file with the changes from #9 for the latest dev. dont tested it yet.

neRok’s picture

deciphered’s picture

Status: Needs review » Needs work

While 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_callback has 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_function function 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.

neRok’s picture

This 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;

  1. Integrated the small change I outline https://drupal.org/node/2119789#comment-8575177
  2. Added an additional group to the 'extra' token regex. It is group 6, and would normally return ?itok=
  3. Change the preg_replace, with a preg_match_all
  4. Iterate through the preg_match_all list. Identify if the match has a token or not, and replace string with str_replace.

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.

function _filefield_paths_replace_path($old, $new, $entity) {
  // Build regular expression.
  $info = parse_url(file_stream_wrapper_uri_normalize($old));
  $info['path'] = !empty($info['path']) ? drupal_encode_path($info['path']) : '';
  $info['host'] = !empty($info['host']) ? drupal_encode_path($info['host']) : '';
  $absolute = str_replace("{$info['host']}{$info['path']}", '', file_create_url($old));
  $relative = parse_url($absolute, PHP_URL_PATH);
  $regex = str_replace('/', '\/', "({$absolute}|{$relative}|{$info['scheme']}://)(styles/([a-z0-9\-_/]*?)/{$info['scheme']}/|)({$info['host']}{$info['path']})");

  // Build replacement.
  $info = parse_url(file_stream_wrapper_uri_normalize($new));
  $info['path'] = !empty($info['path']) ? drupal_encode_path($info['path']) : '';
  $info['host'] = !empty($info['host']) ? drupal_encode_path($info['host']) : '';
  $replacement_path = $info['host'] . $info['path'];

  // Handle tokens for image styles, if it is present.
  if (defined('IMAGE_DERIVATIVE_TOKEN')) {
    $regex .= '((\?(\S+?&|)' . IMAGE_DERIVATIVE_TOKEN . '=)(\S{8})|)';
  }

  $fields = field_info_fields();
  foreach ($fields as $name => $field) {
    if ($field['module'] == 'text' && isset($entity->{$field['field_name']}) && is_array($entity->{$field['field_name']})) {
      foreach ($entity->{$field['field_name']} as &$language) {
        foreach ($language as &$item) {
          if (preg_match_all("/$regex/", $item['value'], $matches, PREG_SET_ORDER)) {
            foreach ($matches as $match) {
              if ($match[5]) {
                // String needs image style token replaced.
                $item['value'] = str_replace($match[0], $match[1].$match[2].$replacement_path.$match[6].image_style_path_token($match[3], $new), $item['value']);
              } else {
                // String does not have image style token.
                $item['value'] = str_replace($match[0], $match[1].$replacement_path, $item['value']);
              }
            }
          }
          if (isset($item['summary']) && preg_match_all("/$regex/", $item['summary'], $matches, PREG_SET_ORDER)) {
            foreach ($matches as $match) {
              if ($match[5]) {
                // String needs image style token replaced.
                $item['summary'] = str_replace($match[0], $match[1].$match[2].$replacement_path.$match[6].image_style_path_token($match[3], $new), $item['summary']);
              } else {
                // String does not have image style token.
                $item['summary'] = str_replace($match[0], $match[1].$replacement_path, $item['summary']);
              }
            }
          }
        }
      }
    }
  }
}
neRok’s picture

StatusFileSize
new32.84 KB

The 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...

              if ($match[5]) {
                ...
              } elseif ($match[2]) {
                $item['value'] = str_replace($match[0], $match[1].$match[2].$replacement_path, $item['value']);
              } else {
                ...
              }

Or something similar. I haven't got time to test, it's home time!

Dima_N’s picture

Deciphered

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_function function does look promising.

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.

deciphered’s picture

Status: Needs work » Needs review
StatusFileSize
new2.79 KB

Dima_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.

neRok’s picture

I tested it, and it works.

I looked at http://au1.php.net/create_function, which says

Caution This function internally performs an eval() and as such has the same security issues as eval(). Additionally it has bad performance and memory usage characteristics.

I thought I would test the performance. I put a timer_start as the first line of _filefield_paths_replace_path, and a timer_read at 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.

david_garcia’s picture

That'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?

deciphered’s picture

david_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.

david_garcia’s picture

¿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.

deciphered’s picture

Status: Needs review » Needs work

It 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....

zombirus’s picture

#16 works for me

sinn’s picture

#16 works fine

daniel wentsch’s picture

Thanks 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

neRok’s picture

@Daniel Wentsch, you should patch against dev.

sumeet.pareek’s picture

Status: Needs work » Needs review
StatusFileSize
new2.19 KB

Here is the patch by @Deciphered in #16 rerolled against the module version 7.x-1.0-beta4

interdruper’s picture

Status: Needs review » Reviewed & tested by the community

Patch #16/#26 works fine for me.

Fidelix’s picture

I tested it as well.

Patch applies to current stable version and fixes the issue.

gkelly’s picture

I added patch #26 to version 7.x-1.0-beta4 and the issue was resolved. Thanks.

daniel wentsch’s picture

Thanks a lot!

rudiedirkx’s picture

Works for me too. Any time on a new release? It's been 17 months since beta4.....

kolier’s picture

StatusFileSize
new2.57 KB
kerios83’s picture

@Sumeet.Pareek #26

Thanks! I have tested it and it's working very well.

+1 for a release.

francescosciamanna’s picture

#26 worked like a charm(Thx)! What about include the patch at least in the Dev. Release?

mlecha’s picture

Using patch #26 to version 7.x-1.0-beta4. Works. Thank you.

dxx’s picture

Ok, patch applied successfully with current --dev version.

osman’s picture

Patch in #32 applies to 7.x-1.x-dev without any issues.
Fixes regex related warnings.

+1 RTBC

Thanks,

rodrigoaguilera’s picture

This module looks pretty unmantained. I think the first step should be for someone to become co-mantainer first

W.M.’s picture

Patch at #26 works perfectly on latest stable beta release. Tested under PHP 5.6.2.

alauddin’s picture

confirm path #26 for version 7.x-1.0-beta4 works.. PHP 5.5.18

zanselm5’s picture

Whenever 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...

kclarkson’s picture

#32 applied cleanly to from the most recent git 7.x-1.x branch.

and yes this module needs some maintainer love.

jimsmith’s picture

#32 worked for me as well. Thanks for the patch, @kolier.

nmillin’s picture

Patch in #32 applies to 7.x-1.x-dev without any issues.
Fixes warnings.

giorgosk’s picture

#32 works as advertised

plazik’s picture

#32 works for me too.

skin’s picture

#32 works for me too.
Tested on version: 7.x-1.0-beta4

joelpittet’s picture

Priority: Normal » Major

RTBC++ bumping to major.

wOOge’s picture

Confirmed — Patch #32 works.

matsbla’s picture

#26 worked for me, thanks! :)

mxr576’s picture

I'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.

mxr576’s picture

(Duplicate comment)

askibinski’s picture

#26 works for me (tested against beta4)

  • Deciphered committed 7f5074d on 7.x-1.x
    #2103151 by Deciphered, david_garcia, Sumeet.Pareek, kitikonti, kolier:...
deciphered’s picture

Status: Reviewed & tested by the community » Fixed

Fixed and committed.

web226’s picture

patch #26 on version 7.x-1.0-beta4. Works. Thanks!

deciphered’s picture

@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'.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jay.lee.bio’s picture

FYI, 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.

hubobbb’s picture

#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"

deciphered’s picture

Hi 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

kmajzlik’s picture

Issue tags: +PHP 7.0 (duplicate)