Views handler loading should respect configuration

Okay a bit of progress, but that's all from me for today. I think there are some tests where its hard to tell if the test is a fail because we are truly breaking something, or because the test is trying to avoid having to fully reproduce a real-world scenario in order to keep the test small.

An example is core/modules/views/tests/src/Kernel/Handler/FieldTimeIntervalTest.php. It appears to attempt to override the views data (rather than using the $override or changing the config) and calls some sort of advancedRender() method. I think what we should do for these type of failures is override the config rather than views data within the test method. There are probably quite a few tests that would need to be fixed in order to do that.

So it seems the fix is probably the tip of the iceberg and the tests relying on this are going to be where the real work is here. The question is, is there a real risk we break things in contrib here that do something similar to what the tests are doing? Ie, enough to e.g. only be able to push this in D12 for example.

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?

Okay a more realistic example is BlockExposedFilterAJAXTest::testExposedFilteringAndReset as that's a functional JS test. 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.

I did run our Views Reference Field test suite against this without issues and we do make a lot of modifications there.

@catch I think we might need to defer to you whether to pursue this route further vs go back to have the two options in ✨ Continuation Add Views EntityReference filter to be available for all entity reference fields Active rather than trying to replace in place.

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.

Okay I created 📌 Make the comments in Views handler Manager around overrides clearer Needs review for the '@ this is cra...' comment (which probably we anyways should not use this specific wording). Hopefully that's a simple/quick fix.

For the automated tests, its going to be a big job as there are tons of cases. I will try to slowly plow through them but please do expect a quite large change to views tests directory. So please do flag if you feel they need to be broken into separate issues. I think it probably makes sense to keep them here as they are best revealed by this code change in ViewsHandlerManager.

Also opened 📌 Try to remove the override functionality from views handlers Active to try to find out how bad it is.

Thanks for the feedback on the progress so far! Replied to comments, will continue to work on this

I also added the related issues that will block this issue.

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.

Spun off issue here 🌱 [meta] Decide how to manage test view configuration so it remains up to date Active . Will try to get my head back into this for sorting out this particular issue.

I guess continuing is probably better than waiting to figure out a way to automatically solve it as it is probably going to be complicated as the Views config will change depending on which other modules are enabled at the time. We could look at module/view dependencies, but as your diff shows, we cannot trust that the view config dependencies were correctly set up in the first place either.

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.

Started work on this again. As an FYI this will get blocked by 🐛 Views content language field default configuration should use field_language plugin Needs review as we will need that to get the tests here to pass I think.

Okay tests all passing now. Finally got around to fixing the views config for those two failing functional JS tests, both outdated views config.

I wonder whether we can add a deprecation that detects when a views data/views data alter is attempting to change the plugin ID used.

If there is a contrib module relying on views data/views data alter to provide an alternative filter plugin ID already, anyone who has exported their config should be fine as the plugin_id saved should be the altered one. They would however need to change their approach for future installs which could be for example:

• Install hook that changes plugin_id
• Wait for ✨ Configurable views filters to allow for different widgets Active and let users select their plugin_id

So this puts pressure on getting the latter in. If anyone has an examples of contrib modules that rely on views data/views data alter to provide a custom handler plugin would be helpful to be able to test.

Note that we still have 🐛 Views content language field default configuration should use field_language plugin Needs review to get in which was flagged to do as a separate issue in the MR comments which could use a nudge back to RTBC (if agreed its ready for that)

Should this be postponed on 🐛 Views content language field default configuration should use field_language plugin Needs review ?

Hmmm I can imagine there is going to be a lot more back and forth in review here. Technically yes it needs to wait but I expect this particular issue to need a fair bit longer so ideally it gets reviewed first to get the back and forth ball rolling.

Just committed 🐛 Views content language field default configuration should use field_language plugin Needs review

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.

scott_euser → changed the visibility of the branch 3458099-views-handler-10-3-x--DO-NOT-MERGE to hidden.

Just backporting to 10.3 to easier test on existing sites. Still need to address the feedback in #23 (thanks for the feedback!)

To note, this seems to have the pleasant side effect of fixing the 'Broken filter handler' issue that there are various related issues in the drupal.org issue queue for over the years (though all seem to be different scenarios). We have found that any 'MySQL Server has gone away' resulting in broken filter handler no longer occurs when configuration is respected.

Anyways nice side effect of more reliable Views in heavy sites!

Sorry for the delay here, was focused on a bunch of contrib projects for a bit! Okay test only should fail now again, I had typed the alter hook name wrong, whoops!

There seem to be some unrelated test failures that have nothing to do with Views after merging latest from 11.x. They pass locally when I rerun... maybe @smustgrave you've seen similar on your travels through the issue queue lately? For now triggered rerun again.

Yep tests passing now after rerun

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 ✨ Configurable views filters to allow for different widgets Active now!

catch → closed merge request !8588

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 📌 Deprecate hook_hook_info() Active .

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.

acbramley → reopened merge request !8588

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.

Sorry thanks for the tip, will make sure to do that next time. And thanks for sorting out the deprecation, much appreciated!

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

catch → closed merge request !8588

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 🐛 Outdated plugin_id definitions in default views config breaks on Drupal 11.2 Active

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.

Hmmm yeah sounds like our worries in #4, #6, #8 discussion coming true. Opt in flag per View sounds like a good idea.

New Views can default to opted in perhaps.

Should I open a follow up? Or do revert first and and here?

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.

Yep, we started discussing that over here ✨ Configurable views filters to allow for different widgets Active but have not progressed yet. Makes sense to keep that separate maybe? Seems an opt-in per View is more urgent.

Ah sorry my screen hadn't refreshed and didn't see #53, #54 - interesting idea!

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 suggests but you can still modify your configuration to allow that (or they have to do a views data alter battle to be the last in line to alter?). So it still opens up quite a world of options as is.

An opt in for old views + opt in by default for new views would be much quicker as I suspect ✨ Configurable views filters to allow for different widgets Active is going to be a long haul D12/D13 type feature.

> 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. ✨ Configurable views filters to allow for different widgets Active 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 ✨ Configurable views filters to allow for different widgets Active 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 📌 Allow @FieldType to customize views data Needs work (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.

catch → closed merge request !11360