Fix label token replacement for views entity reference arguments

#148 🐛 Fix label token replacement for views entity reference arguments RTBC did not apply with drupal 10.0.9 and PHP 8.1

This should work for 10.x

#151 🐛 Fix label token replacement for views entity reference arguments RTBC successfully applied for me.

Will follow up if it breaks my website ;)

The test failure is in one of the new tests, so that certainly needs fixing.

+++ b/core/modules/views/views.post_update.php
@@ -37,3 +39,33 @@ function views_removed_post_updates() {
+function views_post_update_views_data_argument_plugin_id() {

Ideally this should use the ViewsConfigUpdater and the update patterns used for that, so we also update any newly imported Views coming from modules and install profiles

But this is looking pretty great otherwise!

Rerolled #122 for 10.1.x and 11.x.

Selected correct version.

The last submitted patch, 155: drupal-2640994-154-10.1.x.patch, failed testing. View results →

I keep running into this bug. Due to attributes vs. annotations, this doesn't backport cleanly, so I opened new branches and MRs for 10.3.x and 10.2.x. Meanwhile, hiding all the old patches in favor of the 3 MRs.

I updated the summary. Hope that's clear enough for review.

Haven't looked into pipeline failures, so leaving at NW.

Let's get https://git.drupalcode.org/project/drupal/-/merge_requests/7091 passing ASAP. I can backport appropriate changes to the other branches / MRs. But they're here for now for other folks like me that need these fixed on 10.2.x sites.

Embedding the screenshots from #149 into the summary, too, so we don't need those from anyone else. 😅

Thanks,
-Derek

Pushed a few more commits to hopefully get more of the pipeline stages passing.

Meanwhile, one of the test failures is a new deprecation test added for 🐛 Return translated term name on views "Content: Has taxonomy term ID (with depth)" Fixed . Since we haven't shipped 10.3.x, can we remove that test entirely? This is probably a sign we need to be more careful about BC? Adding as related.

Addressed #153 and converted the post_update to use ViewsConfigUpdater. However, EntityArgumentUpdateTest.php was still failing locally. Then determined that the fixture that test is using was based on a stale Views schema. I re-created the fixture view on a clean 10.2.x site, and now that test is passing locally. Pushed fixes to all branches. Let's see what the bot thinks now...

Bumping this to major since I believe this qualifies under:

Render one feature unusable with no workaround

https://www.drupal.org/docs/develop/issues/fields-and-other-parts-of-an-... →

The entire feature of setting display titles based on argument values converted to labels is broken, and AFAICT there's no workaround.

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.

Via Slack, @catch pointed me to 📌 Set budgets rather than exact numbers of asset size assertions Downport . Rebased the 11.x and 10.3.x MRs so hopefully that should solve the FunctionalJS failures so far. 🤞

Meanwhile, I started a draft CR: https://www.drupal.org/node/3441945 →

Back to NR...

Bah, the validatable config job is such a PITA. For a few weeks, it was failing to find a valid version of drush to install. Now it's complaining about this:

Installing Drupal
In install.core.inc line 966:
                                                                               
  Resolve all issues below to continue the installation. For help configuring  
   your database server, see the <a href="https://www.drupal.org/docs/install  
  ing-drupal">installation handbook</a>, or contact your hosting provider.<ul  
  ><li>The database server version <em class="placeholder">3.40.1</em> is les  
  s than the minimum required version <em class="placeholder">3.45</em>.</li>  
  </ul>                                                                        

I don't have time for this. 😂 I'm going to ignore that for now, and I hope everyone else does, too. Crossing that off from the summary.

So this is really ready for review now. Would love to see this finally smashed, ideally for 10.3.x. I suspect this is probably too potentially disruptive for a 10.2.x backport, but I'm going to keep that MR up to date since I definitely need this working on some 10.2.x sites.

Thanks!
-Derek

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

I don't know what the bot is doing, but it's confused, so turning it off from this issue.

I realized we don't want to remove ViewsArgumentDeprecationTest, we want to fix it. 😅 I moved the deprecation dance to EntityArgument, so it now supports all the old legacy ways it might be instantiated / used since it's replacing all the other core classes. Updated ViewsArgumentDeprecationTest to trigger all the new deprecations and check for the right ones. I think we should edit the CR from 🐛 Return translated term name on views "Content: Has taxonomy term ID (with depth)" Fixed ( https://www.drupal.org/node/3427843 → ) to only talk about IndexTidDepth since the Taxonomy one is now gone.

The 10_2_x version safely handles all the legacy stuff but throws no deprecation notices.

Meanwhile I updated the CR here with the full explanation of all the new BC stuff: https://www.drupal.org/node/3441945 → Updated the API changes section of the summary.

Hrm, 🐛 [random test failure] Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews random fail Fixed seems to keep happening, or a new problem has arisen, or something. But WidgetViewsTest is randomly failing a lot right now, both on the bot, and locally, with or without this MR branch checked out. 😢

• https://git.drupalcode.org/issue/drupal-2640994/-/pipelines/150738 (for MR !7091 / 11.x) is currently green (other than the validateable config stuff).
• https://git.drupalcode.org/issue/drupal-2640994/-/pipelines/150753 (for MR !7571 / 10.3.x) is similarly green/yellow.
• https://git.drupalcode.org/issue/drupal-2640994/-/pipelines/150748 (for MR !7572 / 10.2.x) has previously been green, but is currently red. I've tried re-running that 1 job (FunctionalJS 2/2) But I don't want to waste too much bot time on this.

Hopefully that doesn't turn into a huge time-sink.

Meanwhile, I opened 1 thread (in 7091) about the only thing left in this MR that I'm concerned about. Feedback and reviews most welcome. I think we're darn close to RTBC now. 😅 Phew!

Thanks,
-Derek

dww → credited kim.pepper → .

Thanks for the reviews! Crediting kim.pepper and Lendude.

Lendude asked about the difference between EntityArgument vs. EntityReferenceArgument. I wasn't sure. 😂 It was introduced at comment #120. I tried to remove the distinction and make everything use EntityArgument, but then Kernel/ViewExecutableTest::testArgumentValidatorValueOverride() started failing (see pipeline job 1377453):

1) Drupal\Tests\views\Kernel\ViewExecutableTest::testArgumentValidatorValueOverride
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    '{{ arguments.uid }}' => '0I[">&1Z'
+    '{{ arguments.uid }}' => ''
     '{{ raw_arguments.uid }}' => '1'
 )

The test view argument definition ( see core/modules/views/tests/modules/views_test_config/test_views/views.view.test_argument_dependency.yml) seems weird:

        uid:
          id: uid
          table: node_field_revision
          field: uid
          relationship: none
...
          entity_type: node
          entity_field: uid
          plugin_id: numeric

Why entity_type: node if this is pointing to a user? I don't fully understand the use of entity_type in existing definitions...

So maybe it's safer (if a little clumsy) as two separate plugins. Although should one extend the other? Or perhaps we could use one plugin, but make sure it uses target_entity_type_id not entity_type?

For now, reverted the commits to unify the two, and instead pushed docs for both. All the relevant tests are passing locally again. The bot agrees, phew.

Meanwhile, per @catch in #d11readiness the 11.x branch is really 11.x now. We definitely need the separate MRs here. I cleaned up the 11.x MR to remove the deprecation tests and BC layer from EntityArgument. All that only lives in !7571 now. The 10.2.x MR (!7572) has the silent BC layer.

Opened 🐛 [PP-1] Use labels in Views argument summaries for entity references Active to help resolve the last of the threads, keep the scope smaller in here, and finish the job that was started at #2912332: Allow entity reference views arguments to display the entity label in titles and summaries → .

I believe this is RTBC, but obviously someone else will have to say so at this point. 😂

Thanks!
-Derek

Can the MRs be cleaned up?

Not sure what you mean. The ones I opened are very clean. 😂 I don’t have edit access to 7091, so the title and description are lacking, but does that matter? The summary explains what all 3 are for.

11.x has consistent functional test failure.

Ahh, thanks. The 11.x MR was failing due to 📌 Remove the 9.4 database dump fixtures from 11.x Active . 😅 Pushed a commit to use the 10.3.0 fixture instead of 9.4.0. That works locally in the 10.3.x MR. My local can't run 11.x ATM. 😂 Also rebased both MRs to latest 11.x and 10.3.x core. We should be all green again. 🤞

Pipelines are all green again, except the functional JS random fail in the 10.2.x MR mentioned above. I doubt the RMs are actually going to backport this to 10.2.x, and that MR is mostly for folks to patch their own live sites with until 10.3.0 is out and they can upgrade to it...

The 11.x branch needed a rebase (with some fairly trivial conflicts) due to 📌 Removed deprecated code in taxonomy module Fixed

@dww posted about this in #bugsmash so came here.

So following the steps to reproduce I was able to see the issue at hand.

Ran the test-only feature, large output so won't paste here, but here's the link https://git.drupalcode.org/issue/drupal-2640994/-/jobs/1409783

Applied a small nitpicky suggestion for a return type.

Applying the MR the issue does appear to be addressed.

The 1 thread got a response, so believe this is good to go.

Thanks for the reviews and RTBC! I pushed the bool return type to ViewsConfigUpdater::needsEntityArgumentUpdate() to both 10.3.x and 10.2.x MRs. Thanks for spotting that.

Applied MR-7572, it looks correct now

alexpott → changed the visibility of the branch 2640994-10_2_x-label-token-replacement to hidden.

The 11.x MR needs work due to changes in the ViewsConfigUpdater... I think we need to trigger deprecations from the ViewConfigUpdater when it is fixing a view during preSave that's not done as part of the post update. This way modules will know that they have to update their shipped content.

Rebased the 11.x MR to resolve conflicts.

@alexpott: Not sure what you mean with this:

I think we need to trigger deprecations from the ViewConfigUpdater when it is fixing a view during preSave that's not done as part of the post update. This way modules will know that they have to update their shipped content.

Sounds like scope creep to me. I don't want to be rewriting ViewConfigUpdater to add new functionality like this to try to fix this bug. Can we address that in a follow-up?

Thanks,
-Derek

I asked in #bugsmash for help on this. @catch pointed me to 📌 ckeditor5 and editor module tests rely on hook_editor_presave() bc layers Active , but that issue has nothing to do with ViewsConfigUpdater. I still don't exactly understand what's being asked of me.

Views is doing this:

function views_view_presave(ViewEntityInterface $view) {
  /** @var \Drupal\views\ViewsConfigUpdater $config_updater */
  $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class);
  $config_updater->updateAll($view);
}

@alexpott, are you proposing we should change the API for ViewsConfigUpdater::updateAll() to take a 2nd argument to determine if we're being called from presave or post_update? Or do you propose that views_view_presave() checks the return value from ViewsConfigUpdater::updateAll() and if it ever gets back TRUE it should always trigger a deprecation?

Can we pretty please move all such complications to a dedicated follow-up, and fix this before the 10.3.0-beta1 ships? I still fail to see why this poor bug needs to be the one to deal with all that additional functionality.

Tentatively moving back to RTBC to get more committer eyes on this.

Thanks,
-Derek

I got a bit more clarity from @catch on what's expected here. I pushed some commits to the 11.x and 10.3.x MRs to trigger deprecations when ViewsConfigUpdater::processEntityArgumentUpdate() changes a view outside of post_update. I'm not sure on the wording of the deprecation message, that could use another pair of eyes. Also unclear if we "need" tests that this deprecation plumbing works as expected. I sure hope that's not my responsibility here. 😅

I'll keep an eye on the pipelines to make sure I didn't break anything, but I hope this is now actually RTBC.

Wow, what a colossal PITA. 😆 Guess it's good I jumped through all those hoops, since:

1. There were indeed some test views with stale argument plugin definitions that needed to be manually fixed, including the view for testing this new functionality. 😂
2. EntityArgumentUpdateTest was making some slightly bogus assertions.

However, I had to completely rewrite ViewsConfigUpdater::processEntityArgumentUpdate() to operate on entire View entities, instead of using all the per-handler plumbing we've got. To my great dismay, if you copy the existing patterns, ViewsConfigUpdater process functions make a bunch of changes to handler config, then try to save the view, but that instantiates another ViewsConfigUpdater object (this time, with deprecations enabled) to do all the work again. 🤯 So apparently, The Right Way™️ to update a view with all this plumbing is that your process function has to directly update the $view (which isn't even normally passed in to your process function if you think you're just dealing with handlers), not just fix the $handler. 🤦‍♂️

I guess I'm now in the exclusive club of ~5 people on Earth who understand how this class works. 😬 😂

ANYWAY, we're back to green pipelines on 11.x and 10.3.x. I've given up on the 10.2.x MR at this point. It works great for the project where I needed this fixed, all the additional hassles are irrelevant to that site, and this is clearly not getting backported. I just hope this can still land in 10.3.x!

Thanks,
-Derek

I'd be lying if I said I fully understand the view config update (not one of the 5 haha) but running the test-only feature the tests for the update are failing as expected

1) Drupal\Tests\views\Functional\Update\EntityArgumentUpdateTest::testViewsFieldPluginConversion
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'entity_target_id'
+'numeric'

Can see the full list of expected test failures here https://git.drupalcode.org/issue/drupal-2640994/-/jobs/1604882

So believe the addition of that is good

catch → closed merge request !7091

Reviewed this all again:

- constructor deprecations in plugins in the 10.3 branch for removal in 11.x - this is OK during beta given it's plugin constructors to fix a major bug.

- config bc layer has a deprecation in 10.3 for removal in 12.x - this is good since it will make the bugfix non-disruptive for contrib modules.

Also thanks for the work on the config deprecation layer, hopefully we can do this correctly for all config updates from now onwards.

Committed/pushed to 11.x and cherry-picked back through to 10.3.x, thanks!

catch → closed merge request !7571

Woo hoo! 🎉 Thanks, @catch!!

I got the follow-up ready if anyone following here is interested in reviewing (and hopefully RTBC'ing) that one:

🐛 [PP-1] Use labels in Views argument summaries for entity references Active

Thanks again! Very happy to see this fixed.

p.s. Added 10.3.x versions to the CR and published it:
https://www.drupal.org/node/3441945 →

smustgrave → credited darrenwh → .

Closed 🐛 Exposed contextual taxonomy filter does not update title Closed: duplicate as a duplicate and moved over credit.

Is there a follow-up issue for how to fix contrib tests that are now broken when the views .yml definition is updated to use the new plugins?

-          plugin_id: numeric
+          plugin_id: entity_target_id
+          target_entity_type_id: user

Tests pass at 10.3 but fail at 10.2 with

Schema errors for __the_view_name_ with the following errors:
display.user_page.display_options.arguments.uid.not
missing schema,

user_page.display_options.arguments.uid.target_entity_type_id
missing schema

More details on 📌 Fix: The update to convert "numeric" arguments to "entity_target_id" is deprecated Active

Replied on 📌 Fix: The update to convert "numeric" arguments to "entity_target_id" is deprecated Active but also this is a general problem and one we might hit more often as config validation improves so it'd be good to document the outcome somewhere.

I also replied there. But for here, TL;DR: the problem isn’t really about strict schema validation. It’s that the plug-ins do not exist in 10.2.x. So until folks can drop 10.2.x support, they’ll have to live with the deprecation warnings in 10.3.x and up that the auto-conversion will be dropped in 12.0.x.

Should we do a quick follow up to silence this deprecation in 10.3+ so folks don’t have to worry about it until they’re on 11.x?

Should we do a quick follow up to silence this deprecation in 10.3+ so folks don’t have to worry about it until they’re on 11.x?

I think that would be a very good idea, because currently this is preventing testing at 10.3. The default views provided by a contrib project are loaded in every phpunit test, not just the tests which use the views. So I currently have the scenario which I think is wrong on both sides:

• If I ignore all deprecations when testing at 10.3 I cannot tell what needs to be fixed
• If I do not ignore deprecations at 10.3 every phpunit test contains this deprecation warning and it is very hard to read through the noise to see the other deprecations.

I am using the core ignore file in the gitlab CI job

SYMFONY_DEPRECATIONS_HELPER: "ignoreFile=$CI_PROJECT_DIR/$_WEB_ROOT/core/.deprecation-ignore.txt"

Maybe there is a way I could patch that file, or add more deprecations to ignore in a second file, or some other way?

There are many new deprecations in 10.3. They are added continuously to the next minor version. You can't expect to be deprecation free for the next minor version, or even the current version if you want to keep support for earlier versions. Sometimes there are workarounds, with if/else constructs, DeprecationHelper and what not, but for some deprecations, that just not possible or only with a lot of effort (you could probably do some magic at config install time if you find a hook/event/wrapped service that's early enough)

That's not a problem, that doesn't need to be fixed. There are no expectations to be deprecation-free at any point, it's an early warning system that something will change in the future, nothing is broken yet. There is a reason that deprecations are off by default in Gitlab CI.

Trying to keep next* test runs deprecation free is just a continuous race against Drupal core development. The only thing you can reliably go for is to be deprecation free on the oldest version you provide support for.

@berdir: Re #212: That's an extremely compelling case for why trying to "chase HEAD" is a headache. Quoting my advice to @jonathan1055 at #3451750-9: Fix: The update to convert "numeric" arguments to "entity_target_id" is deprecated → :

So my recommendation for right now is to leave your views using numeric, tell GitLab CI to ignore the resulting depreciation notices until you’re ready to ship a release that requires at least 10.3.x, then you can convert your shipped views and reconfigure your pipelines to fail on depreciations (if that’s what you actually want). 😅

However, I'm somewhat torn. You can't conditionally include different .yml in your views config based on versions. There really is no work-around if you want to try to keep up. There's 0 harm in silencing this deprecation in 10.3.x -- folks have years to learn about it and switch. I'm a big fan of making it as easy as possible to be as compatible with as many versions as possible, since ultimately, that's best for end users.

When I pinged @catch in Slack about this (before your comment), he replied:

So leave everything in 10.3 so contrib can actually use the new thing when we get to 10.4?
Silencing or just removing the deprecation in 10.3 seems ok yeah.

So I opened 📌 Remove deprecation about Views argument config conversion from numeric to entity_target_id from 10.3.x Closed: duplicate with a green MR so we can make things easier before the 10.3.0 final release.

Let's continue to discuss there, if necessary, and let this giant issue finally go silent for the 90 followers that only wanted their entity reference arguments to work. 😂

Thanks!
-Derek

Automatically closed - issue fixed for 2 weeks with no activity.