Drupal core 9.3 updated the typecasting for the node_revision route parameter which means it's a fully fledged object now. The code in this module for getting the entity is using code that thinks it's the revision id, a numeric value, which then throw this error:
TypeError: Illegal offset type in Drupal\Core\Entity\ContentEntityStorageBase->loadRevision() (line 636 of core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php).
See attached patch.
User testing
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 3386719-4-revision-object-is-returned-instead-of-revision-id.patch | 844 bytes | franceslui |
| #2 | 3386719-1.patch | 1.08 KB | sahilgidwani |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !3
Comments
Comment #2
sahilgidwani commentedComment #3
sahilgidwani commentedComment #4
franceslui commented@sahilgidwani Thank you for submitting the patch in your comment #2. I have reviewed it and found that it worked correctly.
Since node_revision_load is deprecated in Drupal 10.1.0 and will be removed in Drupal 11.0.0, I have submitted a new patch that uses \Drupal\Core\Entity\RevisionableStorageInterface::loadRevision instead.
Comment #5
joelpittetThank you both, this issue is indeed major as it is a WSOD for content editors. I have reviewed the patches and tested it out and it works well.
Comment #6
damienmckennaThis needs:
* To be recreated as a merge request.
* To be rewritten to use DI.
* Add test coverage.
Comment #7
damienmckennaComment #9
damienmckennaI created a MR from the patch, it's ready for someone else to convert to DI.
Comment #10
joelpittet@damienmckenna DI can be a bit of scope creep like we could do StaticPageSettingsForm as well. I kept the changes localized to avoid that...
Comment #11
dwwThanks for working on this!
Unless we change the minimum supported version, I think it’d be better to do this:
Then we always know we’re getting a vid, not an object, regardless of what core we’re running against. We don’t even want the loaded revision object, we only use it to get the vid back for loading the right revision of the node.
Meanwhile, yeah, I’d rather not scope creep this with all the DI changes. We can do those in a follow-up, they’re not necessary for fixing this bug.
Comment #12
dwwIn addition to #11, this also still needs tests. 😅
Comment #13
joelpittet@dww are the DI changes I already did going to be OK? I tried to keep the scope as tight as possible.
Comment #14
dwwPersonally, I'd rather just fix the bug here, and do all the DI stuff at #3563027: Use dependency injection in StaticPageSubscriber and StaticPageSettingsForm. Hopefully fairly easy to move those commits/changes to another branch.
Meanwhile, I opened #3563026: Enable GitLab CI for static_page to add the
.gitlab-ci.ymlfile and fix the pipelines. That's now merged. So this needs a rebase to remove that from here.Thanks again for working on this!
-Derek
Comment #15
dwwComment #16
dwwMoved the DI changes to #3563027: Use dependency injection in StaticPageSubscriber and StaticPageSettingsForm. Rebased to latest 8.x-1.x. There's now test coverage here, removing that tag, too.
Let's see what the bot says now.
Comment #17
joelpittetThe tests-only need the schema issue #3563033: Missing config schema and default config to show if they are actually testing the right thing. I'll have to get ddev-drupal-contrib locally to test local better.
Comment #19
dwwI merged #3563033: Missing config schema and default config. Rebased in here to remove the schema changes that are now in 8.x-1.x. Pipeline is happy. Merged to 8.x-1.x.
Thanks, everyone!