Problem/Motivation

In Layout Builder we have

*   context_definitions = {
 *     "entity" = @ContextDefinition("entity"),
 *     "view_mode" = @ContextDefinition("string", required = FALSE),
 *   }

with an accompanying plugin alter hook that does

  $definitions['overrides']->getContextDefinition('entity')
    ->addConstraint('EntityHasField', OverridesSectionStorage::FIELD_NAME);

Ideally we would be able to do something like

*   context_definitions = {
 *     "entity" = @ContextDefinition("entity")->addConstraint('EntityHasField', OverridesSectionStorage::FIELD_NAME),
 *     "view_mode" = @ContextDefinition("string", required = FALSE),
 *   }

or even

*   context_definitions = {
 *     "entity" = @ContextDefinition("entity", ['EntityHasField' => OverridesSectionStorage::FIELD_NAME]),
 *     "view_mode" = @ContextDefinition("string", required = FALSE),
 *   }

Proposed resolution

Add handling for constraints to \Drupal\Core\Annotation\ContextDefinition allowing annotations like this:

 *   context_definitions = {
 *     "entity" = @ContextDefinition("entity", constraints = {
 *       "EntityHasField" = \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::FIELD_NAME,
 *     }),
 *     "view_mode" = @ContextDefinition("string", required = FALSE),
 *   }

Remaining tasks

Write tests

User interface changes

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
FileSize
781 bytes

With this patch, this works

 *   context_definitions = {
 *     "entity" = @ContextDefinition("entity", constraints = {
 *       "EntityHasField" = \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::FIELD_NAME,
 *     }),
 *     "view_mode" = @ContextDefinition("string", required = FALSE),
 *   }

Because only annotation namespaces are loaded, the fully-qualified class name is need for that constant, even though its for the file we're in.

EclipseGc’s picture

yeah, I totally love that. The FQCN was to be expected. We've done this rodeo before heh.

Eclipse

tim.plunkett’s picture

The last submitted patch, 4: 3016420-constraints-4-FAIL.patch, failed testing. View results

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community
tim.plunkett’s picture

Title: Explore allowing context definition annotations to specify constraints » Allow context definition annotations to specify constraints
Status: Reviewed & tested by the community » Needs review
Issue tags: +Blocks-Layouts
FileSize
4.59 KB
1.82 KB

The code we wanted this for landed, so rerolled to include that.

EclipseGc’s picture

Status: Needs review » Reviewed & tested by the community

What an enormous improvement.

tim.plunkett’s picture

+++ b/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php
@@ -22,7 +22,9 @@
+ *       "EntityHasField" = \Drupal\layout_builder\Plugin\SectionStorage\OverridesSectionStorage::FIELD_NAME,

It's a shame that the FQCN has to be repeated even within this file (instead of static::FIELD_NAME or something), but that's to be expected with the way that annotations are parsed.

tim.plunkett’s picture

alexpott’s picture

Issue summary: View changes
Issue tags: +Needs change record

We should have a change record to tell developers they can do this.

alexpott’s picture

Issue summary: View changes

I was trying to think of a release note snippet but I'm not sure that

Context definition annotations can specify constraints.

is worth it :) it is a rehash of the title.

tim.plunkett’s picture

tim.plunkett’s picture

Reroll, no changes.

tim.plunkett’s picture

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community

That's a really nice DX improvement! Change record makes perfect sense.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 2dfe7b6d36 to 8.8.x and e716af79b9 to 8.7.x. Thanks!

  • alexpott committed 2dfe7b6 on 8.8.x
    Issue #3016420 by tim.plunkett, EclipseGc: Allow context definition...

  • alexpott committed e716af7 on 8.7.x
    Issue #3016420 by tim.plunkett, EclipseGc: Allow context definition...
alexpott’s picture

Issue tags: +8.7.0 release notes
alexpott’s picture

Issue tags: -8.7.0 release notes

Oopps yeah no release notes.

Status: Fixed » Closed (fixed)

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

bkosborne’s picture

It's too late now, but one downside to the approach taken is that it's not possible to add multiple constraints of the same type in the annotation. I guess using the plugin alter hook would still work though, and it's probably a bit of an edge case (though one that I hit).

bkosborne’s picture

Hmmm I assumed I could alter block plugin defintions, but I don't see a hook for that anywhere. I just found this 7 year old issue where it was apparently added as a way to alter any plugin definition, but that code isn't there any more... https://www.drupal.org/project/drupal/issues/1705702

bkosborne’s picture

Ooookay, sorry for the noise everyone. Looks like it's actually a limitation of ContextDefinition. The constraints are stored in an array keyed by constraint name, so it wouldn't be possible to have multiple constraints of the same name anyway.