Problem/Motivation

When writing a custom Views Display Extender plugin, you may need to validate it before submission of view edit form. Validation of plugins is performed in the validate() method of the DisplayPluginBase class. However, there is no validation of extender plugins in this method.

Proposed resolution

Add display extenders validation in DisplayPluginBase::validate().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

WalkingDexter created an issue. See original summary.

WalkingDexter’s picture

Patch with proposed resolution.

dawehner’s picture

Status: Active » Needs review
Issue tags: +Needs tests

Nice! Do you mind expanding the test in core/modules/views/tests/src/Kernel/Plugin/DisplayExtenderTest.php / \Drupal\views_test_data\Plugin\views\display_extender\DisplayExtenderTest??

WalkingDexter’s picture

Issue tags: -Needs tests

The changes relate to the validation of the view, and not to the display extenders. Testing the validation of the view is already implemented in \Drupal\Tests\views\Kernel\ViewExecutableTest.

borisson_’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Either way, we need tests to ensure that this behavior doesn't regress over time.

WalkingDexter’s picture

Status: Needs work » Needs review

Ok, what tests are needed? I mean, if we expand the test \Drupal\views_test_data\Plugin\views\display_extender\DisplayExtenderTest, then what functional needs testing? Just checking that the validation of display extender returns correct errors?

borisson_’s picture

#6 yes. Just checking that we return the correct errors is exactly what we need :)

WalkingDexter’s picture

Added testing of validation.

Status: Needs review » Needs work

The last submitted patch, 8: views-display-extenders-validation-2967567-2.patch, failed testing. View results

WalkingDexter’s picture

Status: Needs work » Needs review
FileSize
2.53 KB

Added testing of validation [2].

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I think the tests here look really solid and it looks like it tests the correct thing.

Lendude’s picture

Do we need a CR for this? Since this could 'break' existing sites (well they would have been broken, but silently, and after this change, it won't be silent anymore).

catch’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Let's add the change record, CNW for that and tagging.

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.

WalkingDexter’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record

Created a change record => https://www.drupal.org/node/2986918

andypost’s picture

Status: Needs review » Reviewed & tested by the community

Looks great to go

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed b659cbd and pushed to 8.7.x. Thanks!

  • alexpott committed b659cbd on 8.7.x
    Issue #2967567 by WalkingDexter, borisson_, catch, dawehner, Lendude:...

Status: Fixed » Closed (fixed)

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