Views text area plugin does not declare a dependency on the text format it uses

I was looking at this patch because of #2928351: Make sure filter dependency is in the help topic configuration, where we need to do something similar.

Just as a note... the calculateDependencies() method is supposed to add dependencies to $this->dependencies and then return $this. See
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21...

That is most likely why your tests failed on this patch...

Oh wait, sorry, that is for config entities and this is a plugin... may not apply...

+++ b/core/modules/views/src/Plugin/views/area/Text.php
@@ -14,6 +16,43 @@
+  public function __construct(array $configuration, $plugin_id, $plugin_definition, EntityTypeManagerInterface $entity_type_manager) {

Should we deprecate calling this without an EntityTypeManager?

I converted the patch in #2 to an MR. Then I fixed it because the return value from calculateDependencies() was incorrect. I added a test. At some point I realized that this needed an update path. So I wrote that and added a test for it too.

I did not address the question in #13. I wasn't sure if it's necessary since plugins are @internal.

Change applies cleanly, test pass and the correct dependency is now added to the exported config. I think this is OK to go.

Probably need to update any default views that ship with core to include these. Example views.view.frontpage.yml in node, there may be others.

Results of grep -rn "plugin_id: text$" --include views.view.*.yml:

core/profiles/demo_umami/config/install/views.view.frontpage.yml:303:          plugin_id: text
core/profiles/demo_umami/config/install/views.view.frontpage.yml:373:          plugin_id: text
core/modules/content_translation/tests/modules/content_translation_test_views/test_views/views.view.test_entity_translations_link.yml:104:          plugin_id: text
core/modules/views/tests/fixtures/update/views.view.test_filter_format_dependencies.yml:164:          plugin_id: text
core/modules/views/tests/fixtures/update/views.view.test_entity_id_argument_update.yml:193:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_empty.yml:34:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_display_empty.yml:40:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_id_argument.yml:160:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml:109:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_tokens.yml:141:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:50:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:56:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:148:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:154:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:161:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_destroy.yml:167:          plugin_id: text
core/modules/views/tests/modules/views_test_config/test_views/views.view.test_token_view.yml:205:          plugin_id: text
core/modules/views_ui/tests/modules/views_ui_test/config/install/views.view.sa_contrib_2013_035.yml:178:          plugin_id: text

Not all of those views specified the content of the text, so there was nothing to update in those cases. But I updated the ones that needed a dependency.

Believe all views in core have been addressed, double checked that views.view.front in node actually didn't have any format use also (just to follow up on #22)

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Restoring RTBC status

It even has an upgrade path test. Excellent.

Saving issue credits.

Regarding #15:

Should we deprecate calling this without an EntityTypeManager?

I think since it has a factory method, BC-and-deprecation for the constructor might not be necessary. But I will confirm.

OK, I asked in committer Slack about the constructor deprecation, and got feedback from @larowlan:

I would ask for it - even though the policy doesn't require it.
It takes us not much effort and makes minor to minor updates less painful

...Which is what I'm more comfortable with anyway; I was just trying not to be over-restrictive or NW unnecessarily in case we had become less restrictive about this since I last worked the RTBC queue. 😀

So let's add a default null value and a deprecation to the constructor. However, it does not require a deprecation test, nor a change record mention of the added service.

...Though, we probably should have a change record of the added config dependency. Generally, anything that needs an upgrade path also wants a CR.

Thanks everyone!

I added the change record. I haven't addressed other feedback yet.

As always, @mstrelan, a really great catch!

The question about what to do in calculateDependencies() with invalid data gets us into subsystem/release/framework manager review territory. I can't decide which and I'm two of the three, so let's put these two tags on for now, to make sure we take a close look at the decisions we make about this before an eventual commit.

First, though:

1. Let's address the deprecation stuff to get that out of the way.
2. Let's get folks' feedback/info on the thread from @mstrelan's point on calculateDependencies().

Then, we can decide exactly what to do about calculateDependencies() and what we need to add for handling existing broken views.

Thanks everyone!

This is still a bug, and it caused a bug in Canvas/Drupal CMS: #3566119: [upstream bug] `views.view.canvas_pages` triggers validation error when installed outside Drupal CMS: "Missing text format: content_format.".

Responded to @xjm's Q in #32: https://git.drupalcode.org/project/drupal/-/merge_requests/11568/diffs#n...

Per https://git.drupalcode.org/project/drupal/-/merge_requests/11568/diffs#n..., we probably want to wait to do this until #2536594: Add a FilterFormatRepository providing methods to load filter formats lands, which is currently RTBC.

Looked into updating/fixing this MR. But then I noticed this probably should be postponed.

#2536594: Add a FilterFormatRepository providing methods to load filter formats is in, so I think this is unblocked.