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().

CommentFileSizeAuthor
#10 views-display-extenders-validation-2967567-3.patch2.53 KBwalkingdexter
#8 views-display-extenders-validation-2967567-2.patch2.37 KBwalkingdexter
#2 views-display-extenders-validation-2967567.patch653 byteswalkingdexter

Comments

WalkingDexter created an issue. See original summary.

walkingdexter’s picture

walkingdexter commented
StatusFileSize
new views-display-extenders-validation-2967567.patch653 bytes

Patch with proposed resolution.

dawehner’s picture

dawehner
Primary language German
commented
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

walkingdexter commented
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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
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

walkingdexter commented
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

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

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

walkingdexter’s picture

walkingdexter commented
Issue tags: -Needs tests
StatusFileSize
new views-display-extenders-validation-2967567-2.patch2.37 KB

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

walkingdexter commented
Status: Needs work » Needs review
StatusFileSize
new views-display-extenders-validation-2967567-3.patch2.53 KB

Added testing of validation [2].

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
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

lendude
Primary language Dutch
Location Amsterdam
commented

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

catch
he/him
Primary language English
commented
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

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

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

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Reviewed & tested by the community

Looks great to go

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
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.