Follow-up to #2454439: [META] Support PHP 7

From the error on DrupalCI it looks like PHP's get_defined_functions() is not deterministic - so we need to sort.

CommentFileSizeAuthor
#6 2600282-6.patch2.46 KBdawehner
#5 interdiff.txt639 bytesdawehner
#5 2600282-5.patch11.99 KBdawehner
#2 2600282-2.patch1.83 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
FileSize
2600282-2.patch1.83 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented

On PHP7 06:55:00 Drupal\system\Tests\Update\UpdatePostUpdateTest 217 passes - yay!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Needs review » Reviewed & tested by the community

Looks great.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2600282-5.patch11.99 KB
interdiff.txt639 bytes

Well, I had that discussion the other day and alex was not really motivated to touch the phpunit tests of this particular class.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2600282-6.patch2.46 KB

Well javascript testing is nice, but not that important.

The last submitted patch, 5: 2600282-5.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/lib/Drupal/Core/Update/UpdateRegistry.php
@@ -112,6 +112,8 @@ protected function getAvailableUpdateFunctions() {
+    // Ensure that the update order is deterministic.
+    sort($updates);

So I read the method to see if this was correct and not undoing any dependency sorting or whatnot. Turns out all getAvailableUpdateFunctions() does is look for any functions named MODULE_post_update_NAME where MODULE is in the enabled modules and NAME is, well, like, anything. :) So there is no reason not to make this fix.

It took me longer to figure this out than it should because for some reason the post_update bit of that is abstracted into UpdateRegistry::$updateType but only the one value is ever used. See #2538108-26: Add hook_post_update_NAME() for data value updates to reliably run after data format updates, comments 26, 31, and 34.

Anyway, this seems like a sensible fix. Reminds me of #2420421: HEAD BROKEN: Fatal error: Cannot use Drupal\Component\Utility\Xss as Xss because the name is already in use in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views/src/Plugin/views/field/Field.php on line 11.

Committed and pushed to 8.0.x. Thanks!

  • xjm committed 71e0f19 on 8.0.x 
    Issue #2600282 by dawehner, alexpott: PHP7 fails on Drupal\system\Tests\...

Status: Fixed » Closed (fixed)

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