Views handler loading should respect configuration

The question is, is there a real risk we break things in contrib here that do something similar to what the tests are doing

I think that's definitely a concern, as well as custom sites overriding plugins in data_alter hooks without updating their config... How would we deal with that?

In that case the config used by that test is at views.view.test_block_exposed_ajax.yml which has the plugin_id for content type set to 'in_operator' and that causes the select to have Yes No instead of the list of content types. Again it could be that this is because we have created the config manually, and a real-world scenario editing the View would have saved the correct plugin ID.

That seems very likely to me, seems like a result of hand-editing.

It is possible that a contrib or custom module has updated filter plugin IDs without a corresponding update to update config, but it would have to do that without the views ever having been resaved. A bit hard to tell without some real-world testing on long-established sites probably.

Also opened #3458298: Try to remove the override functionality from views handlers to try to find out how bad it is.

See MR discussion. Uploading a modified views.view.test_serializer_display_field.yml from rest_test module and also the diff vs. 11.x

I had to call a couple of methods from ViewTestData, install views_test_data module, edit the view, save it, export it, run the test, get config validation errors, hack out the extra keys, import it again, and that fixes the initial test failure for StyleSerializerEntityTest.php (while introducing another one which I haven't looked into yet).

This confirms that these views exports are incredibly old and have not been maintained over time, so we are testing invalid views exports everywhere. Not really surprising but a shame this issue that's already been split from a 500+ comment two part issue is the one dealing with it.

I think we should open another spin-off issue to discuss how we could maintain these properly over time. Config validation might help, but it doesn't solve the problem of having to manually import, save, export the views.

Yeah I have no idea how to solve automating this, especially with views_test_data because you can't even just install the module to get it usable, have to create state entries first, but good to have an issue open in case someone else has ideas.

Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies.

Should this be postponed on #3459496: Views content language field default configuration should use field_language plugin ?

Just committed #3459496: Views content language field default configuration should use field_language plugin

What's a good way to test this one?

Also any concern validation went down?

As we discussed the validation probably went down because views isn't fully validated yet so adding more view config probably made it go down.

I did run the test-only change and unfortunately it passed

The task

Add automated test case to verify this

still appears to be needed.

Seems test coverage mentioned in #23 has been resolved

1) Drupal\Tests\views\Kernel\ModuleTest::testViewsGetHandler
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'Drupal\views\Plugin\views\filter\BooleanOperator'
+'Drupal\views\Plugin\views\filter\NumericFilter'
/builds/issue/drupal-3458099/core/modules/views/tests/src/Kernel/ModuleTest.php:120
FAILURES!
Tests: 7, Assertions: 85, Failures: 1.

Resolved the threads as they appeared to be addressed or answered

Applied the MR and did a fresh install and views appears to install just fine, wasn't sure how else to test but believe this one is good

Even though this is a bugfix, there could be modules or sites relying on the bug, so I've added a change record. Would be great to double check for accuracy.

https://www.drupal.org/node/3501838

Also made the release notes snippet a bit more verbose and tagging for that.

I did a couple of initial exploratory commits here but it wasn't really writing proper code, so I think I'm still eligible to commit this one.

I think we may be unblocked on #3438054: Configurable views filters to allow for different widgets now!

This caused a head failure - just a deprecation and easy to fix, we need to move the views data include into the test module or ideally convert it to an OOP hook. See #2233261: Deprecate hook_hook_info groups, mark hook_hook_info() for deletion.

Reverting, but please ping me if you rebase and I'll try to commit again asap - will be a trivial fix and neither this issue nor the other issues are trivial.

Sorry for the noise, took a bit to figure out what was going on. The views_test_data module can't be enabled as part of the $modules static in StyleSerializerEntityTest as that means the module gets installed with no schema and blows up tests. This is done in $this->enableViewsTestModule instead. Not sure if this somehow changed recently or not but we're green again :)

Edited some of the comments in the test in the MR to improve clarity.

Oh yeah enableViewsTestModule is a fun one I went down a deeeep rabbit hole in the conversion issue trying to figure out what was failing and it turned out to be a hook that was documented wrong.

@oily good updates for the comments.

At first I wasn't sure how that deprecation was missed, but it's cause that file was new in this MR so that makes sense.

@acbramley, generally it's preferred to rebase rather than merge, but I see @scott_euser previously merged in several times.
@scott_euser, not sure why there were merges here, it does make tracking these a bit harder.

I reviewed all of the commits since the revert (aside from the 11.x merge) and they all look good.
The pipeline is green, I did a quick review of the whole MR and it looks good.

Trying again. Committed/pushed to 11.x, thanks!

Double checked the pipelines and while there were other random failures/database issues etc., this seems to be green in HEAD now. Worst case is it's introduced a random test failure somehow though so should try to keep an eye on it.

@scott_euser no problem, most people follow merge workflows.

For rebasing locally these are the steps I follow:

1. Ensure your local is up to date with the issue fork branch
2. git fetch
3. git rebase
4. Make sure your local 11.x version is up to date
5. git checkout 11.x (this should be tracking origin)
6. git fetch
7. git rebase
8. Switch back to the issue fork you are working on
9. git checkout <branch name>
10. Rebase onto 11.x
11. git rebase 11.x
12. This may be different than merging
13. Resolve conflicts appropriately, note that in a rebase --ours and --theirs are reversed
14. Once a file is resolved you want to add it
15. git add <file or files>
16. DO NOT COMMIT
17. run git rebase --continue (the interface may ask you to commit at this stage)
18. Repeat the conflict resolution steps as many times as you need to
19. Once rebasing is complete you can push it up
20. git push drupal-<issue-number> --force-with-lease
21. The force with lease ensures that you don't overwrite any changes since you last pulled down since this push will change history

Feel free to ping me on slack if you have questions

@acbramley, generally it's preferred to rebase rather than merge

@nicxvan the rebase had conflicts from the first commit and I had limited time. Merging is much easier and in this case doesn't pose any extra effort in reviewing IMO.

@acbramley, that is ok, I was mostly typing that out trying to track down where that deprecation got missed cause I thought something had been lost in the merging / rebasing and it became a bit of a sidebar.

Turns out it was unrelated, it was a new deprecation on head.

Just for the record, this is definitely happening in the wild, see #3505357: Outdated plugin_id definitions in default views config breaks on Drupal 11.2

It was extra confusing in my case because it actually causes an exception on installing the module, combined with weird logic in \Drupal\views\Plugin\views\field\EntityField::getFieldStorageDefinition(). The plugin_id in the default config is a field plugin, while the custom one that was added (many years ago) instead through hook_views_data() is not. So the views config doesn't have a field_name/entity field definition and while all the checks in there check if field_name is set, the definition itself does not, so results in a php notice (which aborts on tests, so you don't get to see the exception) and doing it manually results in this:

 [warning] Undefined array key "field_name" EntityField.php:398

In EntityField.php line 398:

  No field storage definition found for entity type tmgmt_job and field  on view tmgmt_job_overview

I understand why this was done, but it's kind of a behavior change as well (https://xkcd.com/1172) and combined with that exception and possibly other edge cases, this is going to cause some serious problems. I'll need to do a release of TMGMT and if people don't update to that *before* updating to 11.2, they might run into errors that could be hard to recover from.

I think I'm also seeing a second instance of this in the redirect_404 module which I noticed but didn't look into yet, which is causing serialization errors as it's using the serialized plugin on non-serialized data.

I wonder if this needs some kind of BC layer, something like an opt-in flag per view or a global one.

I had a better idea in the meantime I think. We add support to explicitly register multiple supported plugin ids and only respect the config if that matches.

We can also add support for deprecated ids and deprecate using non allowed ones and later on disallow.

That should give us tools to proceed with various views improvement issues.

We can then as also build a UI to configure and change the current handler on top of that.

> An opt in for old views + opt in by default for new views would be much quicker

It *sounds* quick and easy. But who sets that flag, how do you know that the views or even all views of a site are OK? For a global flag, all it takes is a single contrib module that has a misconfigured default config and your site might break in pretty awkward ways (an exception during config install results in a very messy situation).

> I suppose the downside of that is people cannot manually opt-in then by changing their configuration - ie, there is no UI to choose something other than what views data/views data alter

IMHO that makes sense. views data always was the single source of truth for how views can be built, it makes sense to me to keep it that way. #3438054: Configurable views filters to allow for different widgets will need something like that too (a list of feasible fields/filters for a given table column. There are hundreds of plugins, there is no way we can offer users to pick from any of them.

Since this was only fixed and not yet closed (fixed), I'm reopening this for now and will create a proof of concept for this. we can still decide to revert or close and do it in a follow-up.

I pushed a proof of concept of how this could work, to see how that goes in my project. The deprecated stuff is likely for later, coming up with good deprecation messages and so on will not be trivial, just wanted to show how that could work.

The way I imagine ids and deprecated_ids to work is that deprecated_ids are still supported and runtime and will show as an option in a future UI if currently chosen, but if a non-deprecated option is chosen then those will not show. This is similar to how we handled block visibility conditions.

For example for entity_reference changes, we can then set the default to entity_reference and add the old plugin as deprecated_ids, so existing views will continue to work and users can reconfigure it in the UI for a grace period and the next major version can then remove it and then we either automatically switch to the new default or switch to a broken handler and force you to reconfigure it.

I very much assume that this will fail on tests added by this issue.

This triggered tons of deprecation (which means we have quite a few additional mismatches like this in core, they just weren't bad enough to trigger failing tests but will need to be investigated) and exactly one non-random fail, the one that was added here. Slightly adjusted that to make it pass and removed the deprecation for now.

I think the structure and naming needs more discussion. It's not just the iD that's relevant, but the thing is that the whole array is then configuration for the handler.

For example in the altered case, the test module alters the id from boolean to numeric. But the definition also contains a 'type' => 'yes-no', which obviously doesn't make sense for a numeric filter, and passing it through like that could cause weird problems. Reversing that, we can just add ids => ['some_other_id'] then that other id might very well need it's own definition, so we need to make this more flexible.

Maybe something iike this?

$filter['alternatives']['boolean'] = ['type' => 'yes-no']

IMHO, if we don't find a quick solution here that we can agree on we need to consider to revert this. Sadly deprecations don't show up as test fails, so I didn't go through and manually count them all, but every single kernel and functional test job had at least one failing test due to deprecations.

This issue was split out of a previous issue that was already split into two and over 600 comments in total so it would be a shame to go backwards, however it would also be a shame if a lot of modules or worse sites get broken.

> An opt in for old views + opt in by default for new views would be much quicker

It *sounds* quick and easy. But who sets that flag, how do you know that the views or even all views of a site are OK?

We could add a new key to views config entities, and it could be set in each individual default view - e.g. we could set it for exported core views.

We could also set it for views created or update via the UI - because those are guaranteed to be up-to-date (until they're not because they've been exported as default config).

That would then provide a way to deprecate the old behaviour - trigger a deprecation error when a view doesn't have the flag set and remove support for that in a major, forcing every module to update their shipped views. Really modules should be doing that anyway, and so should core, but it doesn't happen.

Then for modules that have the flag but also mismatching views data, we could add an assert() which would fail hard in tests but not affect production sites. This would find wrongly-updated views as well as correctly updated views that eventually go stale again.

IMHO that makes sense. views data always was the single source of truth for how views can be built, it makes sense to me to keep it that way. #3438054: Configurable views filters to allow for different widgets will need something like that too (a list of feasible fields/filters for a given table column.

Well kind of, but I think we should move towards specifying a type, more like the field/typed data APIs for formatters and widgets, and then base filters on that type, rather than having to have potentially six different filter array keys for every views field in views data, which is already massive.

> We could also set it for views created or update via the UI - because those are guaranteed to be up-to-date (until they're not because they've been exported as default config).

Even ignoring the default config problem, views doesn't update the config reliably. I tested with the redirect_404 view, that currently looks like this:

ddev drush cget views.view.redirect_404 display.default.display_options.fields.count.plugin_id
'views.view.redirect_404:display.default.display_options.fields.count.plugin_id': serialized

That didn't change when I resaved the view. It didn't change when I edited and resaved that specific field. It *only* changed after I removed and re-added that field. That can probably be fixed, but again, the amount of views out there that have invalid plugin_ids due to this behavior, manual editing and other reasons must be huge.

> Then for modules that have the flag but also mismatching views data

Mismatching how? The problem IMHO is that we don't know if the mismatch is good or bad. As it stands now with this committed, the ability to have a mismatch is a feature. We can't assert() because we don't know if the mismatch is on purpose or not. The only way to reliably figure out if a mismatch is valid if we have a source of truth that defines valid options.

I totally see the problem with views data being large. But I don't see how we could change that. Relating to to other plugin types, views data is essentially the plugin configuration, we can't put that in the plugin definition as it can vary. And the views UI needs to be able to efficiently list 100+ possible fields and filters to add to your view, I don't see how we could invert it and use something like isApplicable() on widgets/formatters, I'm also not even sure based on what data they would do that.

That didn't change when I resaved the view. It didn't change when I edited and resaved that specific field. It *only* changed after I removed and re-added that field. That can probably be fixed, but again, the amount of views out there that have invalid plugin_ids due to this behavior, manual editing and other reasons must be huge.

We could set the flag only for views newly created via the UI, and then add it for updating views via the UI in a follow-up.

We could do something like that, but it's a bit unclear to me how it helps us in the longterm.

I'm a bit unclear about the scope of #3438054: Configurable views filters to allow for different widgets as it is. There are kind of two rather different goals I think. One is essentially BEF in core, where a sub-type sounds interesting, but that's just the filter UI then.

The other part is dealing with views issues like entity_reference, datetime as well as #2337515: Allow @FieldType to customize views data (essentially, expanding views/configurable fields integration to base fields), where we need a system that allows us to offer improved plugins (not just the form element, but also impact on queries, settings and so on) but also have a BC system for existing views.

Went to revert this, but things have changed in the meantime, so we'll need a revert MR here in order to do so.

Created a revert MR, conflicted on two different issues, in core/modules/views/tests/modules/views_test_config/test_views/views.view.test_exposed_form_checkboxes.yml (conflicted on removed not enabled user roles) and core/modules/views/tests/modules/views_test_data_alter/src/Hook/ViewsTestDataAlterHooks.php (another issue added a class comment that this forgot to do)

Since this is just a revert ticket.

Committed/pushed the revert to 11.x, thanks!

Back to needs work for #51 onwards, we can recommit with that addressed.