Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
When upgrading from 8.8.0 to 8.9.0 and running drush updb I receive the following error. TypeError: Argument 1 passed to Drupal\views\ViewsConfigUpdater::processMultivalueBaseFieldHandler() must be of the type array, null given,
Any suggestions?
Steps to reproduce
Proposed resolution
In ConfigEntityUpdated, add the ability to report and log errors and to continue the update.
Add a new post update function to views to check for errors and report them.
Remaining tasks
Review
Commit
User interface changes
N/A
API changes
N/A
Data model changes
N/A
Release notes snippet
N/A
| Comment | File | Size | Author |
|---|---|---|---|
| #74 | 3145501-74-9.5.x.patch | 1.84 KB | quietone |
| #74 | 3145501-74-9.4.x.patch | 17.14 KB | quietone |
| #74 | diff-71-74-9.4.txt | 1018 bytes | quietone |
| #71 | 3145501-71-9.5.x.patch | 724 bytes | longwave |
| #71 | interdiff.3145501.60-71.9.4.x.txt | 877 bytes | longwave |
Comments
Comment #2
xjmComment #3
plachDoes this help?
Comment #4
plachComment #5
smustgrave CreditAttribution: smustgrave commentedApplied the patch locally and drush updb is able to complete! Thanks for the quick response @plach
Comment #6
larowlan@smustgrave any chance you have a DB backup from before the update?
If so could you test a scenario for us?
If you have the 8.8 codebase + db backup and resave all your views from the UI and then attempt the update without this patch, does it pass?
Because the error is symptomatic of one of your views being broken.
Yes, we can add the extra guard in, but something is not right in your view and this patch won't solve that, so it may rear it's head again at a later point.
Comment #7
plach@smustgrave:
Ok, before the patch is committed, we need to find the root cause: can you post the full stack trace and the yml export of the offending view?
Comment #8
smustgrave CreditAttribution: smustgrave commentedI have an old DB from 8.8.0 but drush updb ran fine. We have several views not sure which could be the culprit.
This is the full error
> [notice] Update started: views_post_update_field_names_for_multivalue_fields
> [error] TypeError: Argument 1 passed to Drupal\views\ViewsConfigUpdater::processMultivalueBaseFieldHandler() must be of the type array, null given, called in C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docr
oot\core\modules\views\src\ViewsConfigUpdater.php on line 269 in Drupal\views\ViewsConfigUpdater->processMultivalueBaseFieldHandler() (line 337 of C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\module
s\views\src\ViewsConfigUpdater.php) #0 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\src\ViewsConfigUpdater.php(269): Drupal\views\ViewsConfigUpdater->processMultivalueBaseFieldHandler(
NULL, 'field', 'rendered_entity', 'page', Object(Drupal\views\Entity\View))
> #1 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\src\ViewsConfigUpdater.php(143): Drupal\views\ViewsConfigUpdater->Drupal\views\{closure}(NULL, 'field', 'rendered_entity', 'page')
> #2 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\src\ViewsConfigUpdater.php(270): Drupal\views\ViewsConfigUpdater->processDisplayHandlers(Object(Drupal\views\Entity\View), true, Objec
t(Closure))
> #3 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\views.post_update.php(396): Drupal\views\ViewsConfigUpdater->needsMultivalueBaseFieldUpdate(Object(Drupal\views\Entity\View))
> #4 [internal function]: {closure}(Object(Drupal\views\Entity\View))
> #5 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\lib\Drupal\Core\Config\Entity\ConfigEntityUpdater.php(128): call_user_func(Object(Closure), Object(Drupal\views\Entity\View))
> #6 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\views.post_update.php(397): Drupal\Core\Config\Entity\ConfigEntityUpdater->update(Array, 'view', Object(Closure))
> #7 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Commands\core\UpdateDBCommands.php(308): views_post_update_field_names_for_multivalue_fields(Array)
> #8 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\includes\batch.inc(251): Drush\Commands\core\UpdateDBCommands::updateDoOnePostUpdate('views_post_upda...', Object(DrushBatchContext))
> #9 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\includes\batch.inc(196): _drush_batch_worker()
> #10 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\includes\batch.inc(99): _drush_batch_command('35360')
> #11 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Commands\core\UpdateDBCommands.php(166): drush_batch_command('35360')
> #12 [internal function]: Drush\Commands\core\UpdateDBCommands->process('35360', Array)
> #13 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\CommandProcessor.php(257): call_user_func_array(Array, Array)
> #14 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolid
ation\AnnotatedCommand\CommandData))
> #15 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(
Consolidation\AnnotatedCommand\CommandData))
> #16 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\AnnotatedCommand.php(302): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\
Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #17 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Command\Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\C
omponent\Console\Output\ConsoleOutput))
> #18 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Application.php(1005): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Cons
ole\Output\ConsoleOutput))
> #19 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Application.php(255): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object
(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #20 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Application.php(148): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console
\Output\ConsoleOutput))
> #21 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Runtime\Runtime.php(118): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Conso
le\Output\ConsoleOutput))
> #22 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Runtime\Runtime.php(49): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
> #23 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\drush.php(72): Drush\Runtime\Runtime->run(Array)
> #24 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\drush(4): require('C:\\Users\\SMUSTG...')
> #25 {main}.
> TypeError: Argument 1 passed to Drupal\views\ViewsConfigUpdater::processMultivalueBaseFieldHandler() must be of the type array, null given, called in C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\m
odules\views\src\ViewsConfigUpdater.php on line 269 in C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\src\ViewsConfigUpdater.php on line 337 #0 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev
1\docroot\core\modules\views\src\ViewsConfigUpdater.php(269): Drupal\views\ViewsConfigUpdater->processMultivalueBaseFieldHandler(NULL, 'field', 'rendered_entity', 'page', Object(Drupal\views\Entity\View))
> #1 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\src\ViewsConfigUpdater.php(143): Drupal\views\ViewsConfigUpdater->Drupal\views\{closure}(NULL, 'field', 'rendered_entity', 'page')
> #2 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\src\ViewsConfigUpdater.php(270): Drupal\views\ViewsConfigUpdater->processDisplayHandlers(Object(Drupal\views\Entity\View), true, Objec
t(Closure))
> #3 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\views.post_update.php(396): Drupal\views\ViewsConfigUpdater->needsMultivalueBaseFieldUpdate(Object(Drupal\views\Entity\View))
> #4 [internal function]: {closure}(Object(Drupal\views\Entity\View))
> #5 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\lib\Drupal\Core\Config\Entity\ConfigEntityUpdater.php(128): call_user_func(Object(Closure), Object(Drupal\views\Entity\View))
> #6 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\views\views.post_update.php(397): Drupal\Core\Config\Entity\ConfigEntityUpdater->update(Array, 'view', Object(Closure))
> #7 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Commands\core\UpdateDBCommands.php(308): views_post_update_field_names_for_multivalue_fields(Array)
> #8 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\includes\batch.inc(251): Drush\Commands\core\UpdateDBCommands::updateDoOnePostUpdate('views_post_upda...', Object(DrushBatchContext))
> #9 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\includes\batch.inc(196): _drush_batch_worker()
> #10 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\includes\batch.inc(99): _drush_batch_command('35360')
> #11 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Commands\core\UpdateDBCommands.php(166): drush_batch_command('35360')
> #12 [internal function]: Drush\Commands\core\UpdateDBCommands->process('35360', Array)
> #13 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\CommandProcessor.php(257): call_user_func_array(Array, Array)
> #14 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\CommandProcessor.php(212): Consolidation\AnnotatedCommand\CommandProcessor->runCommandCallback(Array, Object(Consolid
ation\AnnotatedCommand\CommandData))
> #15 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\CommandProcessor.php(176): Consolidation\AnnotatedCommand\CommandProcessor->validateRunAndAlter(Array, Array, Object(
Consolidation\AnnotatedCommand\CommandData))
> #16 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\consolidation\annotated-command\src\AnnotatedCommand.php(302): Consolidation\AnnotatedCommand\CommandProcessor->process(Object(Symfony\Component\Console\
Output\ConsoleOutput), Array, Array, Object(Consolidation\AnnotatedCommand\CommandData))
> #17 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Command\Command.php(255): Consolidation\AnnotatedCommand\AnnotatedCommand->execute(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\C
omponent\Console\Output\ConsoleOutput))
> #18 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Application.php(1005): Symfony\Component\Console\Command\Command->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Cons
ole\Output\ConsoleOutput))
> #19 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Application.php(255): Symfony\Component\Console\Application->doRunCommand(Object(Consolidation\AnnotatedCommand\AnnotatedCommand), Object
(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console\Output\ConsoleOutput))
> #20 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\symfony\console\Application.php(148): Symfony\Component\Console\Application->doRun(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Console
\Output\ConsoleOutput))
> #21 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Runtime\Runtime.php(118): Symfony\Component\Console\Application->run(Object(Drush\Symfony\DrushArgvInput), Object(Symfony\Component\Conso
le\Output\ConsoleOutput))
> #22 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\src\Runtime\Runtime.php(49): Drush\Runtime\Runtime->doRun(Array, Object(Symfony\Component\Console\Output\ConsoleOutput))
> #23 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\drush.php(72): Drush\Runtime\Runtime->run(Array)
> #24 C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\vendor\drush\drush\drush(4): require('C:\\Users\\SMUSTG...')
> #25 {main}
> [warning] Drush command terminated abnormally.
In ProcessBase.php line 188:
Unable to decode output into JSON: Syntax error
TypeError: Argument 1 passed to Drupal\views\ViewsConfigUpdater::processMultivalueBaseFieldHandler() must be of the type array, null given, called in C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\cor
e\modules\views\src\ViewsConfigUpdater.php on line 269 in Drupal\views\ViewsConfigUpdater->processMultivalueBaseFieldHandler() (line 337 of C:\Users\SMUSTGRA\Sites\devdesktop\cms-dev1\docroot\core\modules\
views\src\ViewsConfigUpdater.php).
Comment #9
plachCan you try this patch? It will output the offending view name.
Comment #10
smustgrave CreditAttribution: smustgrave commentedComment #11
larowlanthe issue is this
rendered_entity: nullUnder displays -> page -> fields
Does that persist if you resave from the UI?
Do you have your config under version control (e.g. git)
if so, can you do some git forensics on that line?
Comment #12
smustgrave CreditAttribution: smustgrave commentedI checked the view and realized I had a broken handler. Once I removed refreshed an old DB and the update ran fine.
Comment #13
larowlanOk, let's bump this down then
Thanks @smustgrave
Comment #14
plachThanks for all the info @smustgrave!
Comment #15
catchSo skipping the bad views config, with a log message like "Unable to update views configuration for $view->id, please review the view in the Views UI and resave it, checking for issues such as broken handler" in a log message seems like all we can do here.
Comment #16
alexpottShould we consider catching exceptions in all the public needs* methods in ViewsConfigUpdater and logging? Or maybe we should do this in the associated post update functions because then we could return the messages as well so the user will see them after the update.
So for example \views_post_update_field_names_for_multivalue_fields() becomes something like:
Comment #17
xjmI still think this is at least major; no matter how we fix this upgrade path it still surfaces more issues.
Comment #18
mxwright CreditAttribution: mxwright as a volunteer commentedI agree with @xjm, I just ran into this error on drush updatedb and had to find this issue and work through the patches before identifying my problematic view and (hopefully) fixing it. Not an ideal update scenario for 8.9.0.
And agree with @catch on skipping the bad view and identifying it in a log- that would be great.
Comment #19
mxwright CreditAttribution: mxwright as a volunteer commentedDidn't mean to change the priority..
Comment #20
plachI'm not sure why we should block the update on fixing the view: the same fix will be applied even if the view is saved after that the update has run. I think skipping the broken handler + logging should give us a more user/developer-friendly solution.
Comment #21
alexpottSo #16 is not going to work because the error is triggered when $entity->save() is called by \Drupal\Core\Config\Entity\ConfigEntityUpdater::update() - so I think we need to add the ability to continue on a report errors to that. Which is no bad thing because then all config entity updates are going to get this ability.
Here's a failing test case.
Comment #22
alexpottNote this issue should probably end up doing #3121008: Add test coverage for ViewsConfigUpdater as this issue affects 9.x too.
Comment #24
plachDisregard #20, I misunderstood what #15 was suggesting :)
Comment #25
alexpottSo here's an implementation for continuing on even when a view is broken.
While implementing this I became more and more unsure about whether this is a good idea. It feels like we're riding a broken bike and instead of getting off to repair it we're continuing to upgrade other parts while cycling towards a brick wall and the broken part is the brakes and at some point we're going to be close to the wall to stop. Isn't this a bit like disabled modules? An idea that, whilst it sounds great, in practice it turns out to be a data integrity nightmare. Maybe we're better off investing in #3145667: Add a new system report that validates entities?
Comment #27
alexpottFixing the test namespaces so that it can run.
@catch and I discussed this issue...
So the patch attached pretty much implements what @catch wants - we log the view with the error, we continue to update, we inform the user the error has occurred. The one really icky bit is
We need the silenced call to ::doOne because ignoring notices in update tests is really hard. Doing
$this->config('system.logging')->set('error_level', ERROR_REPORTING_HIDE)->save();doesn't work because we hard code errors to appear when in MAINTENANCE_MODE - see error_displayable() - and then we convert all notices to exceptions in \Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware.Comment #28
catchIf we don't use @suppression when saving views/importing configuration, I think it is probably OK to keep the suppression in the update, because site admins will run into it elsewhere.
Looks like this could also include the entity ID too in the message? In other places where we want to link to logs, we've done ::moduleExists() for dblog and add the link conditionally - given this is a data integrity issue I think it's worth the effort.
Comment #29
alexpottThe reason I didn't put the entity ID in this message is that this is a 1 to n problem. Putting a message per failed config entity could possibly completely overwhelm the update results page.
+1 on the conditional link to dblog.
Comment #30
catchCould we just do a list of entity IDs then: "The following %entity_type could not be updated: %entity_ids" or similar. Given it's an error message I don't think we need to bother with correct plural formatting on top of conditional linking, but good to list if we can.
Comment #34
quietone CreditAttribution: quietone at PreviousNext commentedUpdating the patch per the last few comments. Add checking for dblog and adding a list of the entity_ids that fail.
Update page from the test
And watchdog. Note there are now two sets of error message. There are two because
views_post_update_image_lazy_loadwill also fail, unless it is run to allow errors.Comment #35
catchIs there a reason to manually decode the exception instead of using watchdog_exception()?
Otherwise this looks great to me.
Comment #36
quietone CreditAttribution: quietone at PreviousNext commentedalexpott may want to answer that beca
I didn't dig too deep but the try/catch caches a \Throwable and watchdog_exception argument expects an \Exception not a \Throwable, so that causes errors. I then changed to catching an \Exception to see what happens, and now ConfigUpdater fails in the callback function.
Comment #37
catchThrowable vs. Exception makes sense, let's leave it as is. Given all that, I think this is ready to go.
Comment #38
catchTechnically this should block a 9.5 beta, but we don't have a specific tag for that, so let's do this.
Comment #39
alexpottI think this would be better to have a single key of
$sandbox[self::SANDBOX_KEY]['failed_entity_ids']Then this would be
if (!empty($sandbox[self::SANDBOX_KEY]['failed_entity_ids']) {I think this leads to cleaner and more consistent info in $sandbox.
I think $entity can have ConfigEntityInterface
Comment #40
alexpottOh I wrote some of this patch so I should just resolve #39....
Comment #42
quietone CreditAttribution: quietone at PreviousNext commentedFailure in Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton, re-testing.
Comment #43
Spokje- Was reviewed and RTBC-ed by @catch in #37
- Changes by @alexpott in #40 address his own nitpicks in #39.
- Interdiff provided in #40 changes only the addressed issues.
- TestBot returns green after those changes.
RTBC for me.
Comment #44
catchWe need the API addition in 10.x too, attaching a patch that is just those hunks without the 9.x test changes.
Comment #45
longwaveThe @todo can be removed now we have the conditional check on dblog?
Comment #46
SpokjeThe @todo was introduced in patch #27 when there was no if branch for the dblog module.
Not sure what happens with the current code when you have syslog enabled, but it seems we don't worry too much about that elsewhere in core.
I'm happy for it to be removed. I'll do so, you can RTBC and a core committer can decide which version of the patches to use?
Comment #47
Spokje10x version with the @todo mentioned by @longwave removed and a follow-up issue mentioned in the remaining todo.
Comment #48
longwave9.5.x patch also has the @todo, but I guess it doesn't matter too much.
Comment #49
catch+1 to removing the @todo, I can remove on commit for 9.5 (if I remember at the time).
Comment #50
SpokjeWorking on the 9.5 patch between bites of (TV-)dinner :)
Comment #51
SpokjeSame for 9.x, interdiff is a bit wonky since my patch doesn't want to do the copy like the original one.
Comment #52
longwaveLooks good to me.
Comment #55
catchCommitted/pushed to 10.1.x, cherry-picked to 10.0.x and 9.5.x, thanks!
Since this can prevent updates from 8.9, I think we should consider backporting it to 9.4.x too, but since there's a (small) API addition, tagging for extra opinions.
Comment #57
longwaveDiscussed the possibility of backport to 9.4.x with @catch and @alexpott. We are in favour, especially given that sites can still go from 8.8 to 9.4, but then when re-reviewing the patch I spotted this:
I think this change was made in error. We should not remove or rename post update hooks once they have been set up; however we cannot revert this now from 9.5.x without breaking sites that upgraded to beta1 or beta2. Fortunately, there should be no harm done if this update is run twice. Therefore I propose keeping this as-is, but also re-adding the original
views_post_update_field_names_for_multivalue_fields()back as a no-op to solve this in both 9.4.x and 9.5.x. This should be added to 9.4.x before it can be backported, and a separate small patch is needed for 9.5.x to add it there as well.Comment #58
catchBumping up to critical to add the new stub update.
Comment #59
quietone CreditAttribution: quietone at PreviousNext commentedApologies for the extra work. Thanks to @longwave for the text for the doc block.
Comment #60
quietone CreditAttribution: quietone at PreviousNext commentedSilly me. Need to remove extra space from summary comment line.
Comment #62
quietone CreditAttribution: quietone at PreviousNext commentedTest failure is from Drupal\Tests\ckeditor5\FunctionalJavascript\MediaLibraryTest::testButton, which is not listed at #2829040: [meta] Known intermittent, random, and environment-specific test failures, retesting.
Comment #63
catchBoth the 9.5 and 9.4 patches look right to me.
Comment #64
alexpottWe also need to decide what to do with 10.0.x I think the answer is to add
views_post_update_field_names_for_multivalue_fields_followuptoviews_removed_post_updates)_.Comment #65
catch#64 is correct we should add it to removed post updates in 10.x
Comment #66
quietone CreditAttribution: quietone at PreviousNext commentedYes, I just didn't get to this yesterday.
Comment #67
longwaveThe problem with adding it to removed post updates now is that, as the tests show, you can no longer upgrade from 9.4.0 to 10.0.0; we either need to skip this change in 10.0.x or make a call that 9.4.9 will be the minimum version to upgrade from (and update the fixtures and do all the other associated work with this change).
For me I think skipping the change in 10.0.x is fine, as long as the user has run
views_post_update_field_names_for_multivalue_fieldswe can assume that they either ran it before this commit or they have run it following this commit and also gotviews_post_update_field_names_for_multivalue_fields_followupat the same time.Comment #68
alexpott@longwave would another option be to not rename the update. We've not released this yet so if we don't rename it then everything is fine right? I guess we renamed it to have it run again - but if it failed the first time why do we need to rename it. It got renamed in #34 with no discussion as to why.
Edit: linked to wrong comment where update rename was introduced.
Comment #69
longwaveIf by "not rename" you mean not rename in 9.4.x, and then revert the rename in 9.5.x? So:
_followupupdate, and gain the original version back, but it doesn't matter if they run this twice (and technically we don't need to support this, but also we shouldn't break their upgrade path unless we have to)So this is probably the best course of action?
Comment #70
alexpottYep #69 is exactly what I mean
Comment #71
longwaveImplemented #68/69.
Comment #72
catchDon't really want to muddy the waters further but there's one issue with #68/#69.
If we add an update to 9.4, but don't add it to 9.5/10.0, then users who update to that version of 9.4, but no other users, will have that update logged as having run permanently in their system. If we ever add back an update with the same name later, the later update won't run.
So a variation on #68/69 would be the following:
1. Add the new update as a no-op to 10.x, then it can be moved to removed post updates in 11.x
2. Add it to 9.5.x as well as 9.4.x.
We don't have a way to clear out old removed post updates from the entire system either, but if we eventually do one day, this would help for consistency there too.
Comment #73
quietone CreditAttribution: quietone at PreviousNext commentedI forced (don't wait for branch to pass) the tests on 9.4 in #71 to run. Although, locally that fails because views_post_update_field_names_for_multivalue_fields is included in the post updates that have run.
Comment #74
quietone CreditAttribution: quietone at PreviousNext commentedThat's odd, I did not set this to NW.
This removes 'views_post_update_field_names_for_multivalue_fields' from the list of existing update functions which allows the update test to run successfully.
Comment #77
quietone CreditAttribution: quietone at PreviousNext commentedAll the failure bar one are functionaljavascript test which are not having a good day today. The other one is from 9.4.x and is also checking an element, which seems to be a common problem in the random failures.
Therefor, setting this to NR
Comment #78
longwave@catch
views_post_update_field_names_for_multivalue_fieldsalready exists in 9.4.x and is marked as removed in 10.0.x.views_post_update_field_names_for_multivalue_fields_followupexists in 9.5.x.If we commit #74 then this puts 9.5.x back in sync by renaming
views_post_update_field_names_for_multivalue_fields_followuptoviews_post_update_field_names_for_multivalue_fields. However, the only users that will have run the removed_followupupdate is anyone who has upgraded to a beta or dev release of 9.5.0. Is this worth worrying about?Comment #79
catchOK I got confused by #69 but I think I get it now:
1. We remove the renamed update from 9.5 because it only ever landed in a beta.
2. We backport the fixes for the update to 9.4, but there is no 'new update' anywhere, just the existing one that was already there.
No changes necessary for 10.x
In which case the worst that can happen is 9.5 beta sites have some cruft in key/value, but it's by far the simplest option.
Comment #81
catchHave been hoping someone else would RTBC this so I could commit it, but it's been about a week, so maybe if I RTBC it someone else can either commit it or +1.
Comment #82
longwaveRTBC+1 though I posted #71.
Comment #84
catchCommitted/pushed to 9.5.x and 9.4.x respectively thanks!
Comment #85
effulgentsia CreditAttribution: effulgentsia at Acquia commentedSince this is no longer blocking beta or RC, removing those tags.
I'm tagging this for release notes in case we think it's important to inform site owners about this there. Though I'm not totally up to speed on this issue, so if release notes aren't needed, we can remove those tags.
Comment #86
catchI think the one thing we could mention in release notes is that if you're stuck on Drupal 8.8, you could try updating directly to Drupal 9.4, so you're running the fixed version of this update instead of the not-fixed version in 8.9.x.
Something like:
According to https://www.drupal.org/project/usage/drupal there are still over 40,000 sites on <= 8.8.
Comment #87
longwave