Problem/Motivation

Follow-up to #2961822: Support object-based plugin definitions in ContextHandler.

As of that issue, object-based plugin definitions can implement ContextAwarePluginDefinitionInterface so that they can manage context definitions in a nice, object-oriented way. ContextAwarePluginBase also generally is able to take advantage of this neat new API.

However, there's a major hole in the implementation: the getContextDefinition() and getContextDefinitions() methods from ContextAwarePluginBase are still completely unaware of ContextAwarePluginDefinitionInterface, and therefore will blow up with a fatal error if the plugin definition is an object which implements that interface.

Proposed resolution

Fix ContextAwarePluginBase so that getContextDefinition() and getContextDefinitions() can handle object-based plugin definitions that implement ContextAwarePluginDefinitionInterface.

Remaining tasks

Write tests, review, commit.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#13 interdiff-2982626-7-13.txt566 bytesphenaproxima
#13 2982626-13.patch3.79 KBphenaproxima
#7 2982626-7.patch3.79 KBphenaproxima
#7 2982626-7-FAIL.patch1.9 KBphenaproxima
#2 2982626-context-2.patch1.89 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
Status: Active » Needs review
FileSize
2982626-context-2.patch1.89 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Looks great but I think tests are needed here :)

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue summary: View changes
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2982626-7-FAIL.patch1.9 KB
2982626-7.patch3.79 KB

And, a unit test!

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Title: ContextAwarePluginBase needs to use ContextAwarePluginDefinitionInterface » ContextAwarePluginBase is incompatible with ContextAwarePluginDefinitionInterface
Category: Task » Bug report

Reclassifying this as a bug report.

The last submitted patch, 7: 2982626-7-FAIL.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

EclipseGc’s picture

EclipseGc CreditAttribution: EclipseGc at Acquia commented
Status: Needs review » Reviewed & tested by the community

This seems super reasonable and frankly about time for us to do this. RTBC.

Eclipse

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Needs work 
FILE: .../Drupal/Tests/Core/Plugin/Context/ContextAwarePluginBaseTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 53 | ERROR | [x] The closing brace for the class must have an empty
    |       |     line before it
----------------------------------------------------------------------
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs work » Reviewed & tested by the community
FileSize
2982626-13.patch3.79 KB
interdiff-2982626-7-13.txt566 bytes

Fixed. Sending back to RTBC since it's a whitespace change.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Issue tags: +blocker

This now blocks #2976148: Layout-based entity rendering should delegate to the correct section storage instead of hardcoding to either defaults or overrides.

  • Gábor Hojtsy committed 9e18995 on 8.7.x 
    Issue #2982626 by phenaproxima, tim.plunkett, EclipseGc:...

  • Gábor Hojtsy committed a3e4724 on 8.6.x 
    Issue #2982626 by phenaproxima, tim.plunkett, EclipseGc:...
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed, thanks! Looks good!

Status: Fixed » Closed (fixed)

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