Come together with the global Drupal community in Rotterdam, 28 Sept – 1 Oct 2026. Sessions, contribution, connection, and Early Bird savings until 8 June.Problem
Edit module uses a custom made-up way to upcast URL arguments from the request. Drupal core got standard upcasting features in the meantime in #1798214: Upcast request arguments/attributes to full objects, so edit should use that.
Proposal
Slightly extend the core upcasting feature, so it can take an entity type and an entity as separate variables and upcast from there.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | edit-upcasting-9.patch | 6.94 KB | gábor hojtsy |
| #9 | interdiff.txt | 1.01 KB | gábor hojtsy |
| #8 | edit-upcasting-8.patch | 6.63 KB | gábor hojtsy |
| #8 | interdiff.txt | 4.28 KB | gábor hojtsy |
| #3 | edit-upcasting-3.patch | 5.72 KB | gábor hojtsy |
Comments
Comment #1
gábor hojtsyAll right I took the patch from @effulgentsia at #1798214-158: Upcast request arguments/attributes to full objects and just slightly modified it (added plenty comments and fixed some spacing). I was looking for test coverage for this in the codebase to update but could not find. This would need to be done for #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits as well.
Comment #2
gábor hojtsyAdded quick test coverage. Looking at #1798214: Upcast request arguments/attributes to full objects obviously helped me find the test coverage's location :)
Comment #3
gábor hojtsyHeh, that was a bit too much code :D Another patch crept in. The interdiff was authentic though.
Comment #4
wim leersI"m not the right person to review this.
I can provide nitpicky feedback though :) Sorry, Gábor!
No spaces around the slash?
We can use strict comparison here, so why not?
Again, why not strict?
Comment #6
gábor hojtsyI did not put the slashes without spaces, so it may be clearer I'm not saying these should immediately follow each other in the URL. :) I can remove the spaces though.
Comment #7
wim leersMakes sense :) Nitpickfail :)
Comment #8
gábor hojtsyAll right, so that review comment ignited some thinking for me though that maybe supporting only the order of entity_type/entity is a limitation. We'd need to have added error handling code for it (the code in prior patches just assumed people would follow docs and fail if you had the wrong order if there is an unknown entity type, making Drupal attempt to get a storage controller for an unknown entity type).
The code below assumes the entity type is vetted, so we should make sure it is vetted even if there was no entity_type key in the path or the value provided there was wrong (even if it came AFTER the 'entity' key). So I added some very little code for that case, so we can now support the params in any order, it can be node/12 or 12/node whatever. Added tests to test for that too.
Also fixed the fail which was due to a copy-paste issue :D
Comment #9
gábor hojtsyAnd while I'm there, I should add tests for an invalid entity type anyway :) This proves there are no problems, the entity type list limitation works well.
Comment #11
gábor hojtsy#9: edit-upcasting-9.patch queued for re-testing.
Comment #13
wim leers#9: edit-upcasting-9.patch queued for re-testing.
Comment #14
fubhy commentedWhile the patch is perfectly good code-wise it moves even further down the same magic routing rabbit hole that we are currently trying to drag ourselves out of again in #1943846: Improve ParamConverterManager and developer experience for ParamConverters.
So if this currently blocks the progress of the Edit module I'd say "Commit it" but be aware that it is VERY likely that we will have to completely change it again later. I want to solve this with #1837388: Provide a ParamConverter than can upcast any entity. which has been sitting there for quite some time without any comments but I am sure there are many places where 'interdependent upcasting' would be useful. Anyways, I am thinking of something that would go hand-in-hand with the effort from #1943846: Improve ParamConverterManager and developer experience for ParamConverters like so:
Comment #15
gábor hojtsyWell, this is needed as much for #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits that we'd need to add a couple more classes to intervene in to handle the path for that patch (which mostly copy-paste existing code from elsewhere). If this is a workaround and that is a workaround, we can just use that workaround. I don't want to step on people's toes (and make blockers for my own issues that don't progress specifically).