The action.post_update.php file is wrongly named as "action.post_udate.php".

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT created an issue. See original summary.

mglaman’s picture

Status: Needs review » Reviewed & tested by the community
Parent issue: » #2815379: Move message, goto, and email action plugins to Drupal\Core\Action

Fixes the filename. This means 8.7.0 shipped with a broken update.

Lendude’s picture

There is an update test for this: \Drupal\Tests\system\Functional\Update\MoveActionsToCoreTest

Why didn't that catch this? Probably means that test isn't testing what we think it does, so we might want to address that too.

mglaman’s picture

Status: Reviewed & tested by the community » Needs work

That test only checks for regression not that anything updated.

https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/system...

It needs to be updated to prove there's a problem.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

slasher13’s picture

Status: Needs work » Reviewed & tested by the community

Does this really need tests? I think it should be fixed in Drupal 8.8

Lendude’s picture

Well the question is more, what is \Drupal\Tests\system\Functional\Update\MoveActionsToCoreTest testing, because it is not testing the update, because if it would, it would fail, because the update has never run.

So the problem here is not just the wrong file name, but also a test that is not testing what it should, I think both should be addressed here, because otherwise we have an update with no update coverage.

Dinesh18’s picture

patch looks good to me. +1 to RTBC

Sam152’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
793 bytes
1022 bytes

This is NW for the issues identified in #7 and #4.

I had a look and the test is fine, it's just some other update between drupal-8.bare.standard and HEAD recalculates the dependencies. Moving to the 8.4 fixture solves the problem. It's not perfect since the same thing could happen again with a later update, but it does at least demonstrate a failing test for the issue today.

The last submitted patch, 9: 3053656-9-TEST-ONLY.patch, failed testing. View results

Lendude’s picture

Status: Needs review » Reviewed & tested by the community

@Sam152 excellent! Nice sleuthing. Now I agree with the RTBC :)

mglaman’s picture

+1 on the RTBC! good to see the test is running now

jibran’s picture

Assigned: Unassigned » catch
similarity index 100%
rename from core/modules/action/action.post_udate.php

rename from core/modules/action/action.post_udate.php
rename to core/modules/action/action.post_update.php

@catch I think we should just commit this change to D9 so that we don't have to reroll #3087644: Remove Drupal 8 updates up to and including 88** with test change. What do you think? Assigning to @catch for the answer.

alexpott’s picture

Assigned: catch » Unassigned

It's worth noting that the post update re-saves certain actions

/**
 * Moves action plugins to core.
 */
function action_post_update_move_plugins(&$sandbox = NULL) {
  $resave_ids = [
    'action_goto_action',
    'action_message_action',
    'action_send_email_action',
  ];
  \Drupal::classResolver(ConfigEntityUpdater::class)->update($sandbox, 'action', function (ActionConfigEntityInterface $action) use ($resave_ids) {
    // Save entity to recalculate dependencies.
    return $action->isConfigurable() && in_array($action->getPlugin()->getPluginId(), $resave_ids, TRUE);
  });
}

So running this is harmless.

@jibran I think you mean the opposite of

I think we should just commit this change to D9

. But the point is moot because that patch is not removing so it needs a reroll to remove core/modules/action/action.post_udate.php so removing core/modules/action/action.post_update.php is no different.

alexpott’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed and pushed 868feb2ba8 to 9.0.x and ce6f10f79b to 8.9.x. Thanks!

  • alexpott committed 868feb2 on 9.0.x
    Issue #3053656 by Sam152, JeroenT, Lendude, mglaman: Rename action....

  • alexpott committed ce6f10f on 8.9.x
    Issue #3053656 by Sam152, JeroenT, Lendude, mglaman: Rename action....
andypost’s picture

+1 to port to 8.8 because actions should be resaved between 8.7 and 8.8

alexpott’s picture

Status: Patch (to be ported) » Fixed

Discussed with @catch and we agreed to backport.

  • alexpott committed c6b0f76 on 8.8.x
    Issue #3053656 by Sam152, JeroenT, Lendude, mglaman: Rename action....
xjm’s picture

Priority: Normal » Critical

Belatedly promoting to critical since this was an upgrade path bug.

xjm’s picture

Title: Rename action.post_udate.php to action.post_update.php » Rename action.post_udate.php to action.post_update.php so that the upgrade path runs correctly
Issue tags: +Drupal 8 upgrade path

Retitling to capture the importance of this.

Status: Fixed » Closed (fixed)

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