Filter glossary view by status

Re-rolled.

Updated patch to fix tests.

Adding backport tags.

Fresh export, lets see what this does

Other patch snuck some change in here, ignore #8

Bit of a clean up

Huh, so assertEmpty doesn't allow unset indexes, weird....

Applied patch #12.The status filter is added by this patch.

Before patch;

After patch:

RTBC +1

Fix is simple (add the missing status to the default view).
There's an update path, with a test, so that's great.
Existing sites will end up with a default status filter on this view (as desired).
Code all looks clean and good. No code style violations or anything to complain about.

My only hesitations before RTBC are:

1. Does this need a release note snippet? I don't think a CR makes sense, but we should probably mention this change and the post update's behavior, just so site admins are aware. - Took a first stab at the note with this summary update.
2. We're testing the upgrade path, but should we test the glossary view itself? E.g. make some nodes, some published, some not, open the glossary, and make sure the unpublished ones aren't listed? I just spent some time searching and I didn't manage to find any tests for the node status views filter. I must not be looking in the right places. ;) I don't think we tend to test the default views. Maybe we should?

Otherwise, I think this is RTBC.

Thanks!
-Derek

Thanks @dww.

#15.1 Release snippet makes sense, something to alert admins that their View might be showing a different result set now sounds great.
#15.2 \Drupal\Tests\node\Functional\Views\StatusExtraTest is the somewhat weirdly named explicit test for this filter and it is
less explicitly tested for example in \Drupal\Tests\node\Functional\NodeAdminTest::testContentAdminPages

Thanks @Lendude,

As I mentioned in Slack, \Drupal\Tests\node\Functional\Views\StatusExtraTest is testing the fancy status_extra filter, the "Published or admin" thing. This issue is adding the regular status filter, for only showing published nodes, even to admins. It's this second "simple" filter that I couldn't find any existing test coverage for, which is what's making me nervous.

I'm also thinking that if it's worth the trouble to have an upgrade path test, it seems worth having explicit coverage (at least for access bypass bugs like this) for the default view itself. Maybe that's starting down a slippery slope. But if we're relying on default views to provide more and more functionality in core (yay), and those default views have access bypass bugs (boo), seems we should be adding test coverage as we go, to ensure that the views we ship are secure by default.

@dww yeah you are right about the tests being for the other filter. Did some more digging, and the filter we are adding is just the boolean filter plugin, which has \Drupal\Tests\views\Kernel\Handler\FilterBooleanOperatorTest, which does it's testing with a status field, so that seems pretty good.

On testing the default Views, from a regression testing stand point I'd hope there would be no need. Any change we make to the View we would have to update our test coverage (so we are always just making our test match our config, so our test won't tell us anything) and if we break something and the test for this fails, I would expect (or at least hope) that some test for the component we broke would fail too. But I get your point, it does feel like something we'd want to make sure works. So, yeah, not sure either.

For this fix I'd opt we stick with what we currently do, and not have test coverage for default Views. If that is something we want, we should do it right and make a meta and add it for all of them.

@Lendude Thanks for the additional test digging! Your findings put my mind at ease. ;)

I'm also now convinced about your reasoning on the functional testing of the default views. If we want it (which is still an if), I agree it should be done properly via a meta and a real effort, not weirdly 1-off for this particular bug.

So, all concerns are addressed. #12 is RTBC for 9.2.x and 9.1.x branches.

I agree with the "Needs backport to D8" tag. This should probably land in 8.9.x branch, too. Adding the "Security improvements" tag since this is fixing an access bypass bug, as additional support for the backport.

#12 doesn't apply to 8.9.x (fuzz in core/modules/node/node.post_update.php) and the new test is using the 9.0.0 fixture which obviously doesn't exist in 8.9.x branch. ;) So at risk of confusing things, here's the 8.9.x backport, which is only resolving the fuzz and changing the test to use:

      __DIR__ . '/../../../../../system/tests/fixtures/update/drupal-8.4.0.bare.standard.php.gz',

and to annotate the test with @group legacy. That DB dump contains a bunch of things that trigger deprecation warnings if you run it locally without legacy:

OK (1 test, 180 assertions)
...
Remaining deprecation notices (11)

  1x: Drupal\comment\Plugin\Action\DeleteComment is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\comment\Plugin\Action\PublishComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\PublishAction instead. See https://www.drupal.org/node/2919303.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\comment\Plugin\Action\SaveComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\SaveAction instead. See https://www.drupal.org/node/2919303.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\comment\Plugin\Action\UnpublishComment is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead. See https://www.drupal.org/node/2919303.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\node\Plugin\Action\DeleteNode is deprecated in Drupal 8.6.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\DeleteAction instead. See https://www.drupal.org/node/2934349.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\node\Plugin\Action\PublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\PublishAction instead. See https://www.drupal.org/node/2919303.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\node\Plugin\Action\SaveNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\SaveAction instead. See https://www.drupal.org/node/2919303.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Drupal\node\Plugin\Action\UnpublishNode is deprecated in Drupal 8.5.x, will be removed before Drupal 9.0.0. Use \Drupal\Core\Action\Plugin\Action\UnpublishAction instead. See https://www.drupal.org/node/2919303.
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. The field_tags field on the node.article.default form display is missing it. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. The uid field on the node.article.default form display is missing it. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

  1x: Any entity_reference_autocomplete component of an entity_form_display must have a match_limit setting. The uid field on the node.page.default form display is missing it. This BC layer will be removed before 9.0.0. See https://www.drupal.org/node/2863188
    1x in GlossaryStatusFilterTest::testGlossaryView from Drupal\Tests\views\Functional\Update

The rest of the 8.9.x tests in core/modules/views/tests/src/Functional/Update use @group legacy, so I think that's the right thing. Would be great to have @Lendude confirm this backport is proper. ;)

Thanks again!
-Derek

p.s. I only knew to review this from a post in #bugsmash, so tagging for that, too...

Backport looks good to me.

+++ b/core/modules/node/node.post_update.php
@@ -34,3 +35,29 @@ function node_post_update_configure_status_field_widget() {
+    $view = View::load('glossary');
+    $display =& $view->getDisplay('default');

Something that just occurred to me, the Glossary View could have been deleted from an install, so we should probably check that the load worked and if not, bail early.

Added the check for loading the View, also updated the setting we add in the update to match the actual config you have after exporting.

Minor PHPCS error in the previously. Uploading a new one.

@Lendude re: #19: Good catch! Agreed that's better. Thanks.
@imalabya re: #21: Thanks! That's indeed the fix for the minor PHPCS error in #20. ;)

#21 is back to RTBC for 9.x branches.

Here's a fresh backport for 8.9.x. Interdiffs relative to previous 8.9.x patch (#18) and latest 9.x patch (#21).

Thanks, everyone!
-Derek

Committed/pushed to 9.2.x, backported to 9.1.x and 8.9.x, also tagging for various release notes.

Fixing tag for intervening sec releases.