PHP 7.2 has deprecated the each() function, and this module uses it twice in DiffEngine.php. It can usually be replaced with a foreach().

Comments

bkosborne created an issue. See original summary.

malaynayak’s picture

Assigned: Unassigned » malaynayak
malaynayak’s picture

Assigned: malaynayak » Unassigned
Status: Active » Needs review
StatusFileSize
new788 bytes

Hi @bkosborne,

Please review the patch.

bkosborne’s picture

Status: Needs review » Needs work
StatusFileSize
new836 bytes

No, I don't think that's right. I think we need to preserve the location of the internal array pointer between these two loops. Foreach won't do that.

mrgoodfellow’s picture

Looking at this issue: https://www.drupal.org/project/drupal/issues/2885309
it recommends the code in this patch.

however when patching I get the following error:
Fatal error: Undefined class constant 'USE_ASSERTS' in \sites\all\modules\contrib\diff\DiffEngine.php on line 318

mrgoodfellow’s picture

StatusFileSize
new2.09 KB

Based on this patch: https://www.drupal.org/files/issues/2885309-43.patch
and this issue: https://www.drupal.org/project/drupal/issues/2885309

However it does not appear to function and throws a fatal error:

Fatal error: Undefined class constant 'USE_ASSERTS' in \sites\all\modules\contrib\diff\DiffEngine.php on line 318

chris burge’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: diff-php7-compat-2946139-5.patch, failed testing. View results

bkosborne’s picture

@mrgoodfellow, it seems like you just copied and pasted code from Drupal 8 as a replacement? That's not necessary. Did you try the patch I posted in #4? It should work fine.

joseph.olstad’s picture

Status: Needs work » Reviewed & tested by the community

I just got this too running the diff module with php 7.2.

Patch #4 works perfectly, thanks!

joseph.olstad’s picture

Great work as usual bkosborne,
FYI: php 7.2 support for D7 core is comming in the next release window, (confirmed).

Lets get this one in for this win.

The last submitted patch, 4: diff-php7-compat-2946139-4.patch, failed testing. View results

joseph.olstad’s picture

patch #4 works great
Please commit this and tag a release , php 7.2.x support is for now. php 7.3.x is already out.

  • Alan D. committed 83370ed on 7.x-3.x authored by mrgoodfellow
    Issue #2946139 by bkosborne, malaynayak, mrgoodfellow: Remove usage of...
alan d.’s picture

Status: Reviewed & tested by the community » Fixed

Ta.

bkosborne was totally right about the internal array pointer, foreach resets this each time, but the horrible old code uses one loop to find and break, and the second loop to progress from the last read element after the break.

e.g. each() == [key(), current()] + next().

@joseph.olstad
Sorry, was meaning to give cred in the commit

@mrgoodfellow
Best to check what your doing when posting patches. D8 version of Diff is completely different to D7 and clearly that patch wouldn't have worked at all.

joseph.olstad’s picture

@Alan D , thank you very much!

The only thing more that I could ask for is a tagged release! :) been two years.
php 7.2.x compatibility is worth tagging and publishing a release for.

alan d.’s picture

Not really that much in dev, one minor change, these two PHP 7.2 bugs really

Changes since 7.x-3.3

#2946139: Remove usage of deprecrated each() function for PHP 7.2+ compatability by bkosborne, malaynayak, joseph.olstad (for PHP 7.2)
#3002285: Remove usage of deprecated assert() on string for PHP 7.2+ compatibility by Chris Burge (for PHP 7.2)
#2872108: Split field label and machine name in field table by hass
#2845895: diff_update_7307 assumes the module is enabled. by torotil, Alan D.
#2860650: Assertion failed in _HWLDF_WordAccumulator->addWords() by joelpittet, Alan D.

Maybe around the time D7.61 comes out

joseph.olstad’s picture

Please release diff, 7.61 is comming (very soon)

I have spoken with the core maintainers, this is why I've published a release for features, for php 7.2.x compatiblity, I've already published several other contrib releases that include php 7.2.x compatiblity. It would be nice if this was too.

alan d.’s picture

Just for Joseph ;)

https://www.drupal.org/project/diff/releases/7.x-3.4

Give it a few minutes to publish.

joseph.olstad’s picture

Thanks!

Status: Fixed » Closed (fixed)

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