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
In order to implement the idea in #2976148-5: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides, it needs to be possible to validate an entity context by checking if it has a specific field attached to it.
Proposed resolution
Create a new EntityField validation constraint, which checks if a fieldable entity has a given field attached.
Remaining tasks
Write the patch, write tests, review, commit.
User interface changes
None.
API changes
A new validation constraint will be added to Drupal core.
Data model changes
None.
Comment | File | Size | Author |
---|---|---|---|
#21 | interdiff-2976356-17-21.txt | 5.52 KB | phenaproxima |
#21 | 2976356-21.patch | 5.18 KB | phenaproxima |
#17 | interdiff-2976356-15-17.txt | 692 bytes | phenaproxima |
#17 | 2976356-17.patch | 6.26 KB | phenaproxima |
| |||
#15 | interdiff-2976356-11-15.txt | 603 bytes | phenaproxima |
Comments
Comment #2
phenaproximaHere's an initial patch without tests (yet), to validate (hah!) the approach.
Comment #3
phenaproximaBack to NW for tests.
Comment #4
phenaproximaAnd now with a test!
Comment #5
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedHere's a quick review :)
How about renaming this to
EntityHasField
?Not sure why we need "entity_reference" in there, this constraint doesn't seem to do anything with references.
If the output is an array, shouldn't the method name be
getFieldOptions()
?Shouldn't we expose this state as a constraint violation? Something like "The entity is not fieldable."..
Let's rename this argument to
$field_names
and always pass an array :)Comment #6
phenaproximaAll fixed!
Comment #7
Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commentedLooks great, this is all I got:
I think the feedback was intended as: lets update the constraint to only accept a list of fields instead of both. If we do indeed support both a string and a list, we should have coverage for it.
If we do take that direction though,
EntityHasFields
is probably a better name. I question the need for this at all though, surely it's trivial to add multiple constraints to something in pretty much all contexts where they are used?I like connecting dots here with a
@coversDefaultClass
, which pretty much makes the comment redundant.Super nit, this variable is probably more accurate as
$node_type
. Also any reason not to simply NodeType::create()?Comment #8
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedWe should have an early return before this check, something like:
We could use
NodeCreationTrait
and$this->createNode()
here :)Related to @Sam152's point above, we should use
$this->createContentType()
instead.I'm also wondering about #7.1: does this constraint really need the ability to handle multiple field names?
Comment #9
phenaproximaAddressed all feedback from #7 and #8. I removed support for multiple fields, since I agree that it's probably superfluous (at least until someone asks for it).
Comment #11
phenaproximaWhoops. Had the silly type hint wrong.
Comment #13
gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commentedThis patch would solve the issue.
Comment #14
gawaksh CreditAttribution: gawaksh at OpenSense Labs for DrupalFit commentedComment #15
phenaproximaThis oughta do the trick...
Comment #17
phenaproximaYour move, Drupal CI.
Comment #18
phenaproximaComment #19
phenaproximaThis issue may block #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides. I won't postpone it just yet, but I think it's worth tagging it as such.
Comment #20
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch looks much better now. Just a little note about the test coverage:
It's a bit weird to add this constraint to a data definition of an entity reference field, isn't the constraint supposed to live on the entity type level?
I would suggest to follow the same approach as
\Drupal\KernelTests\Core\Entity\EntityTypeConstraintsTest
for testing this, and use the existingentity_test_constraints
module :)Comment #21
phenaproximaOkay, rewrote the test. This is definitely cleaner and more extensive test coverage. Thanks, @amateescu!
Comment #22
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThat looks much better indeed!
Comment #23
phenaproximaRealized this will probably need a CR, so I wrote one: https://www.drupal.org/node/2980591
Comment #24
catchShould this also check for the field type? Or if not, what is going to check that?
Also this seems like it could potentially help with #2981835: Forums hard-codes the comment_forum comment field, but doesn't validate it.
Comment #25
phenaproximaI think that checking the field type would be the kind of thing we could address in a follow-up. Right now, the constraint is meant to be very simple, and it assumes you know the name of the field to check for, and that you know the expected type already. So I think that we could open an issue to extend this constraint to implement a field type check.
The use-case that gave rise to this issue was from Layout Builder -- it needs to be able to filter plugin definitions by a context definition, and that context definition must match any entity which has a specific field which has a specific, hard-coded name. Therefore, to fulfill that use case, we only need to check for the field itself; we already know what type it will be, if it exists.
Comment #26
phenaproximaOpened a follow-up to add type checking to the constraint: #2982184: Add field type checking to EntityHasField constraint.
Comment #27
catchi'm wasn't 100% on committing this without field type checking, but the layout example uses a very clearly namespaced field, and we already have issues with forum module relying on a field name and doing no checking at all.
So this is an improvement, and phenaproxima updated the change record to note the limitations.
Committed 28be9f7 and pushed to 8.6.x. Thanks!