Fix label token replacement for views entity reference arguments

Bugs can be fixed in 8.0.x.

Except I'm not sure it's a bug, or where that bug is.

Yes it says title there, but this is about the *argument* title, not the term name. And it is whatever the argument plugin happens to implement, which I guess in this case, isn't much.

In short, it's not a token problem, that very likely returns correctly whatever the argument plugin's getTitle()/title() method returns.

Yeah for the custom integrations we had in d6/d7 there have been all kind of custom code. Now we deal with a generic entity reference field, I believe,
in which case there is no such integration. Maybe though for entity reference arguments we might want to provide that possiblity using $entity->label().

In case we want to get that route, we would also invent an update path which converts all the existing one to use the raw ID first.

Discussed this with @dawehner.

The mentioned code is only used for arguments on the taxonomy term table itself and the special node <-> term arguments.

This means we need a new special entity reference argument plugin that is set in core_field_views_data instead of the default numeric plugin which implements title similar to above.

Note that there's also titleQuery() in IndexTid which actually looks a lot better to me because it uses getTranslationFromContext(). We'll need a mix of those two. (a title() method that uses getTranslationFromContext()).

Added new plugin EntityTargetIdArgument and implemented title() method.

We should add a comment to the $data that we're changing (something about providing a argument plugin that has a meaningful title() implementation.

We'll also need a test.

The only close thing that I could find is RolesRidArgumentTest, which uses a test_user_roles_rid test view.

We'll need something like that in taxonomy tests, with a node + term or so. Apparently we have no such test yet or we would have noticed when converting to entity_reference I guess.

Exported the view previously created (manual token test worked). Providing test coverage using this test view, but somehow it does not properly call our plugin. As discussed with Berdir it may be a missing schema bug, I'll do it asap.

It doesn't work because the argument is broken and doesn't do anything. You always see all results.

That's because in your view, you used field_tags. That's from standard.profile. The test uses a different field name, see mockStandardInstall() in the parent. You need to update your view to use a field with that name.

Fixed test with the suggestion above, added a check in form.inc since it seems there is a "PHP Notice" in core.

Test to be run after this #2665202: PHP Notice bug due to the recent #2595613 commit issue is committed.

This is looking pretty good. There are some remarks below.

1. +++ b/core/includes/form.inc
   @@ -206,7 +206,7 @@ function template_preprocess_fieldset(&$variables) {
   -  if ($element['#title_display'] == 'invisible') {
   +  if (isset($element['#title_display']) && $element['#title_display'] == 'invisible') {
   

   What about adding some sane default value in \Drupal\Core\Render\Element\Fieldset instead?

2. +++ b/core/modules/views/src/Plugin/views/argument/EntityTargetIdArgument.php
   @@ -0,0 +1,64 @@
   + * Argument handler for basic taxonomy tid.
   

   This doc needs to have some fix.

3. +++ b/core/modules/views/src/Plugin/views/argument/EntityTargetIdArgument.php
   @@ -0,0 +1,64 @@
   +  function title() {
   +    // There might be no valid argument.
   +    if ($this->argument) {
   +      $entity = $this->entityTypeManager->getStorage($this->definition['target_entity_type_id'])->load($this->argument);
   +      if (!empty($entity)) {
   +        return $entity->label();
   +      }
   +    }
   +    // TODO review text
   +    return $this->t('No name');
   +  }
   

   What about implementing titleQuery() instead and support multiple entity IDs by that automatically?

4. +++ b/core/modules/views/src/Tests/Plugin/EntityTargetIdArgumentTest.php
   @@ -0,0 +1,68 @@
   +    $this->drupalGet('/entity-tid-argument-test/2');
   +    $this->assertTitle('test: title ' . $this->term2->label() . ', input ' . $this->term2->id() . ' | Drupal');
   

   Once we support more than one entity ID, we should also expand the test coverage with a multiple example.

5. +++ b/core/modules/views/tests/modules/views_test_config/test_views/views.view.test_entity_tid_argument.yml
   index 2efc08c..18e0809 100644
   --- a/core/modules/views/views.views.inc
   
   --- a/core/modules/views/views.views.inc
   +++ b/core/modules/views/views.views.inc
   
   +++ b/core/modules/views/views.views.inc
   +++ b/core/modules/views/views.views.inc
   @@ -732,6 +732,12 @@ function core_field_views_data(FieldStorageConfigInterface $field_storage) {
   

   Beside expanding views_views_inc I guess we also want to do the same stuff in EntityViewsData for entity reference fields?

Feedback for @dawehner from #17:

1. that won't be in this patch anymore, because it will be fixed by #2595613: '#title_display' => 'invisible' does not work for composite form elements (see #53)
2. I've checked in other files and edited it like Argument handler to accept an entity target id. or what do you suggest ?
3. I've tried to implement this title() to load multiple entities but it does not work. I'm not sure but I guess it's because as in the previous patch it uses only a single entity, the $titles array that I've returned in title() now is not valid with the required array in Drupal\Core\Render\Element::children() or do I miss something ? How can I fix it ?
4. wait for the point 3. to be done
5. I'm quite new with these stuff and I'm not sure what you want to be done in EntityViewsData, can you give me some more details, please ?

Test will fail since the related issue is not fixed yet.

@tim.plunkett, @effulgentsia @alexpott @xjm @cottser and @dawehner discussed that.
While we agreed that this is annoying, we don't think this is major, as it doesn't affect too many people. This doesn't mean we cannot fix it.
Thank you @tduong for heads up trying to fix it.

Well I would have suggested to overload titleQuery() instead.
On top of that you are now using TaxonomyTerm::loadMultiple(), better use $entity_storage->loadMultiple()

Rerolled. Uploaded patch:

• implemented titleQuery() instead of title()
• set break_phrase: true allowing url with multiple values for the extended test
• extended test coverage for multiple entity IDs examples
• test still fails because of the last assertion (url: /entity-tid-argument-test/1+2), I'll check it tomorrow
• extended EntityViewsData
• the fix in PathPluginBase is temporary because of a bug from a recent commit (#2598502: Double escaping in views attachment titles, see #14)

• fixed EntityTargetIdArgument to get queries with multiple ids either with , or +
• expanded test coverage for all ids combinations possible
• note there still is the temporary fix in PathPluginBase::getRoute()

I've noticed that in a manual test for /entity-tid-argument-test/1+2 the title I get is test: title tax1 + tax2, input 1 2, while in the test the result is title tax1 + tax2, input 1+2. Does someone have any idea which one is the correct one and where it is processed ?

My local instance:

• create some terms
• enable your new vocabulary in Reference Type in /admin/structure/types/manage/article/fields/node.article.field_tags
• create some articles with some terms in Tags
• create views adding field_tags as the contextual filter and override the title: test: title {{ arguments.field_tags_target_id }}, input {{ raw_arguments.field_tags_target_id }}
• if you supply 1+2 as argument in the preview area you'll see "..., input 1 2" instead of "..., input 1+2" like in the test

Fixed test.

Fixed PhpUnit test.

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -527,7 +527,8 @@ protected function processViewsDataForEntityReference($table, FieldDefinitionInt
            ];
            $views_field['field']['id'] = 'field';
   -        $views_field['argument']['id'] = 'numeric';
   +        $views_field['argument']['id'] = 'entity_target_id';
   +        $views_field['argument']['target_entity_type_id'] = $entity_type_id;
   

   This would result in differences exported views as it has a different plugin_id. As we don't have any settings for either, nothing really breaks, but should we write and update function? Is that even possible? Just resaving doesn't help, not until we also fix #2648956: Editing a view leaves old keys hanging, results in invalid schema

2. +++ b/core/modules/views/src/Plugin/views/argument/EntityTargetIdArgument.php
   @@ -0,0 +1,61 @@
   +class EntityTargetIdArgument extends NumericArgument implements ContainerFactoryPluginInterface {
   

   I'm not sure about the name. I originally suggested this, but it's actually not really limited to the target_id column. Maybe EntityIdArgument or just EntityArgument?

   Also, we have large list of argument classes that could either be replaced completely by this new generic argument plugin or at least extend from it (although some extend from others like ManyOne). Many of those are for example missing the translation context thing and would not be displayed in the right translation. An incomplete list:

   * \Drupal\taxonomy\Plugin\views\argument\Taxonomy (is missing the translation context thing)
   * \Drupal\taxonomy\Plugin\views\argument\VocabularyVid
   * \Drupal\taxonomy\Plugin\views\argument\IndexTid
   * \Drupal\taxonomy\Plugin\views\argument\IndexTidDepth
   * \Drupal\user\Plugin\views\argument\RolesRid
   * \Drupal\comment\Plugin\views\argument\UserUid
   * \Drupal\node\Plugin\views\argument\Nid

   We could make a follow-up for those, but if we have to update views configurations, it might be easier to just do them in one go?

3. +++ b/core/modules/views/src/Plugin/views/argument/EntityTargetIdArgument.php
   @@ -0,0 +1,61 @@
   +    foreach ($terms as $term) {
   +      $titles[$term->id()] =\Drupal::entityManager()->getTranslationFromContext($term)->label();
   +    }
   

   minor coding standard issue there.

   If we inject the entity type manager, then we should also inject entity.repository for this method.

   Also, you should rename $terms and $term to $entities and $entity. It's no longer just about terms.

4. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -921,7 +921,7 @@ protected function assertLanguageField($data) {
        $this->assertEquals('numeric', $data['filter']['id']);
   -    $this->assertEquals('numeric', $data['argument']['id']);
   +    $this->assertEquals('entity_target_id', $data['argument']['id']);
   

   you should also check the new target_entity_type_id property that we're adding.

5. +++ b/core/modules/views/views.views.inc
   @@ -735,6 +735,12 @@ function core_field_views_data(FieldStorageConfigInterface $field_storage) {
   +    // Provide an argument plugin that has a meaningful title() implementation
   +    // getting the node label instead of its ID as implemented in
   +    // \Drupal\taxonomy\Plugin\views\argument\Taxonomy::title().
   

   This comment should be updated and shortened and also duplicated into EntityViewsData.

Rerolled.

1) discussed with berdir: wait for more details from @bedir and @dawehner
2) renamed class as EntityArgument/-Test, wait for more details for the rest
3-5) fixed

Discussed with @dawehner:

1. We need at least an update function that clears the views data cache. Views::viewsData()->clear();
2. We move this to a follow-up, please open one .

Created follow up. Added views_update_8005() in views.install, just clear cache line, will work on this tomorrow.

+++ b/core/modules/views/views.install
@@ -347,3 +348,11 @@ function views_update_8004() {
+  // Update views.
+  Views::viewsData()->clear();

Actually an empty update function is enough. The cache is cleared at the end anyway.

.

Painful rebase switching branch to 8.1.x... I saw that meanwhile an update_8005 function has been committed

/**
 * @addtogroup updates-8.1.0
 * @{
 */

/**
 * Clear views data cache.
 */
function views_update_8005() {
  // Empty update function to rebuild the views data.
}

/**
 * @} End of "addtogroup updates-8.1.0".
 */

so I guess it is enough also for us (?)

Let's see first if I've rebased everything right...

so I guess it is enough also for us (?)

I agree its not ideal.

But let's please keep this new update function, there might be people which ran their updates in the meantime.

Sorry, do you mean that I should add also our update function, another empty views_update_8006() ?

Yes, exactly.

Added empty update function 8102 since it is for 8.1.x-dev and it is supposed to be the second update function. Didn't change the number of the first one though to not to break anything.

We no longer needs tests, yeah!

+++ b/core/modules/views/src/EntityViewsData.php
@@ -532,7 +532,10 @@ protected function processViewsDataForEntityReference($table, FieldDefinitionInt
         $views_field['field']['id'] = 'field';
-        $views_field['argument']['id'] = 'numeric';
+        // Provide an argument plugin that has a meaningful titleQuery()
+        // implementation getting the node label.
+        $views_field['argument']['id'] = 'entity_target_id';
+        $views_field['argument']['target_entity_type_id'] = $entity_type_id;

Given that we change the used plugin ID, we should also reference that in the configured views ... as this is will be used for config schema validation, so we need a dedicated update function for that.

Added an update function for configured views plugin ID with entity_reference field type.

Sorry :( I fear we aren't ready yet.

I'm a bit sceptical whether this actually works now ...

+++ b/core/modules/views/src/EntityViewsData.php
@@ -532,7 +532,10 @@ protected function processViewsDataForEntityReference($table, FieldDefinitionInt
         $views_field['field']['id'] = 'field';
-        $views_field['argument']['id'] = 'numeric';
+        // Provide an argument plugin that has a meaningful titleQuery()
+        // implementation getting the node label.
+        $views_field['argument']['id'] = 'entity_target_id';
+        $views_field['argument']['target_entity_type_id'] = $entity_type_id;
         $views_field['filter']['id'] = 'numeric';
         $views_field['sort']['id'] = 'standard';

+++ b/core/modules/views/views.install
@@ -361,5 +363,43 @@ function views_update_8005() {
+ * Update configured views plugin ID.
+ */
+function views_update_8103() {
+  $config_factory = \Drupal::configFactory();
+
+  // Loop through any view, on 'arguments', to update the plugin IDs.
+  foreach ($config_factory->listAll('views.view') as $config_name) {
+    $config = $config_factory->getEditable($config_name);
+    /** @var \Drupal\field\FieldStorageConfigInterface $field_storage */
+    $field_storage = FieldStorageConfig::load($config->getStorage());

This code deals with base fields, but the update function deals with configurable fields. Do we need to do the same kind of change in \core_field_views_data?

This code deals with base fields, but the update function deals with configurable fields.

how should I fix it ?

Do we need to do the same kind of change in \core_field_views_data?

actually we have already provided it in there:

+++ b/core/modules/views/views.views.inc
@@ -735,6 +735,11 @@ function core_field_views_data(FieldStorageConfigInterface $field_storage) {
+    // Provide an argument plugin that has a meaningful titleQuery()
+    // implementation getting the node label.
+    $data[$table_name][$field_name . '_target_id']['argument']['id'] = 'entity_target_id';
+    $data[$table_name][$field_name . '_target_id']['argument']['target_entity_type_id'] = $target_entity_type_id;

or do you mean something else ?

Lol, I was indeed too lazy to scroll to the end of the patch. But still, you need to take into account base fields as well in your update hook.

1. +++ b/core/modules/views/views.install
   @@ -361,5 +363,43 @@ function views_update_8005() {
   + */
   +function views_update_8102() {
   +  // Empty update function to clear the views data cache.
   +}
   +
   +/**
   

   we don't need an empty function if we also add a real update function. drop this and rename the other to 8102

2. +++ b/core/modules/views/views.install
   @@ -361,5 +363,43 @@ function views_update_8005() {
   + * Update configured views plugin ID.
   

   this could be a bit more specific and specificy entity reference argument plugin IDs or so.

3. +++ b/core/modules/views/views.install
   @@ -361,5 +363,43 @@ function views_update_8005() {
   +    /** @var \Drupal\field\FieldStorageConfigInterface $field_storage */
   +    $field_storage = FieldStorageConfig::load($config->getStorage());
   

   Yeah, that doesn't make sense :)

   Whta you need to do instead is loop over the fields and inside, for each key, check if that is a field and if it is an entity reference field.

   Not sure what the best way to do that is. Additionally, it is possible that someone is altering views data to something else.

   Maybe we need to change this to a post update function and then compare it with the actual views data?

Aaah ok, now I get what you meant.. still not sure how to fix it. I'll check it with @Berdir after lunch :)

Added post_update (need implementation), tried to update the views data update function. Still work in progress...

Moved the update function code into the post_update one. Still cannot get the values I need to access the views data, tried to get the views through drupal_get_path() but it seems like there are no results for the modules we need (?) Can anyone help me, please ?

1. +++ b/core/modules/views/src/EntityViewsData.php
   @@ -532,7 +532,10 @@ protected function processViewsDataForEntityReference($table, FieldDefinitionInt
   +        // implementation getting the node label.
   

   let's use entity label here, because this is not just about nodes.

2. +++ b/core/modules/views/views.install
   @@ -361,5 +363,40 @@ function views_update_8005() {
    /**
   + * Just a test update function
   + */
   +function views_update_8103() {
   

   I am confused

3. +++ b/core/modules/views/views.install
   @@ -361,5 +363,40 @@ function views_update_8005() {
   +  foreach ($modules as $module) {
   +    $config_dir = drupal_get_path('module', $module) . '/test_views';
   +    if (!is_dir($config_dir) || !$module_handler->moduleExists($module)) {
   +      $exist = 'nope';
   +      continue;
   +    }
   

   I think we want to iterate over all views using a similar method as above in that file:

       $config_factory = \Drupal::configFactory();
   
   foreach ($config_factory->listAll('views.view.') as $view_config_name) {
   

   This code example here would just update test files, not sure we really want that.

4. +++ b/core/modules/views/views.post_update.php
   @@ -180,5 +180,62 @@ function views_post_update_taxonomy_index_tid() {
   +      foreach ((array) $config->get('display.' . $display_id . '.display_options.arguments') as $argument_id => $argument_config) {
   +        //var_dump($argument_id);
   +        // This update deals with the Entity reference field type.
   +        if (isset($argument_config['type']) && $argument_config['type'] == 'entity_target_id') {
   

   a) Don't you want to check for 'plugin_id' instead of 'type'?

   b) IMHO we should apply these updates ALSO in the case when the configuration used to point to taxonomy arguments?

Trying to do some progress but still cannot get the data that I need... here is just my current test patch

Just a reroll. No conflict encountered.

Ok, trying to get this back on track. Cleaned up the update code, something like this should work but we have no test view, need to ensure one exists first.

does it apply with 8.2?

Here's a test using a similar test view as the one already added but using the existing field_tags field, also renamed a bunch of stuff.

Nice work in general!

1. +++ b/core/modules/views/src/Plugin/views/argument/EntityArgument.php
   @@ -0,0 +1,67 @@
   + */
   +class EntityArgument extends NumericArgument implements ContainerFactoryPluginInterface {
   +
   

   Can we open a general followup to address the fact that entity types might have string IDs?

2. +++ b/core/modules/views/src/Plugin/views/argument/EntityArgument.php
   @@ -0,0 +1,67 @@
   +    foreach ($entities as $entity) {
   +      $titles[$entity->id()] = $this->entityRepository->getTranslationFromContext($entity)->label();
   +    }
   

   I'm wondering whether this should take into account the 'rendering_language' settings in tviews.

3. +++ b/core/modules/views/src/Tests/Plugin/EntityArgumentTest.php
   @@ -0,0 +1,73 @@
   +
   +
   

   Hyper-nitpick: two empty lines

4. +++ b/core/modules/views/tests/src/Unit/EntityViewsDataTest.php
   @@ -1038,7 +1033,8 @@ protected function assertLanguageField($data) {
   diff --git a/core/modules/views/views.install b/core/modules/views/views.install
   
   diff --git a/core/modules/views/views.install b/core/modules/views/views.install
   index 30ac79c..bd21f37 100644
   
   index 30ac79c..bd21f37 100644
   --- a/core/modules/views/views.install
   
   --- a/core/modules/views/views.install
   +++ b/core/modules/views/views.install
   
   +++ b/core/modules/views/views.install
   +++ b/core/modules/views/views.install
   @@ -5,6 +5,9 @@
   
   @@ -5,6 +5,9 @@
     * Contains install and update functions for Views.
     */
    
   +use Drupal\Core\Config\FileStorage;
   +use Drupal\views\Views;
   +
    /**
     * Implements hook_install().
     */
   

   This seems out of scope now.

1. Sure, we can ;) Should we name this class EntityArgumentNumeric then, so we can add an EntityArgumentString later?

2. I had a quick look but I think that is not possible. \Drupal\views\Entity\Render\EntityTranslationRendererBase::getLangcode() depends on a result row, we don't have that here. We could pick th first, but that could be entirely wrong. That entity might not have a translation but the term does. I think this is fine for now.

3. Oh no! How could I? :)

4. I love how dreditor manages to end up with 8x "core/modules/views/views.install" in that snippet ;)

I'm still unsure about doing #30.2 here (using this new class in those existing classes to avoid duplicate code) or not. But I'm going to try that now, we can still move it to a follow-up.

Impressive how much duplicate code we have in those argument plugins. This just removes the code, we could also officially deprecate them and switch the code that specified them directly to entity_target_id (which as this shows, doesn't really make sense anymore as a name).

Most of those plugins are actually used for the ID field of the base table. Which is something that we could now do generically as well. But again, definitely risking going too far here. Deleting all that code is just so much fun. There are even a few more, that could use a string version (RolesRid, Type in node, VocabularyVid is actually incorrectly extending from NumericArgument).

PS: My personal favorite is Fid::titleQuery().

I'll take a look at this...

Re-rolled.

Could you add interdiff?

It is not possible to create an interdiff because it is a relatively complicated re-roll.

1 out of 2 hunks FAILED
interdiff: Error applying patch1 to reconstructed file

Also I managed to forget the new array syntax [] so here's another patch (with an interdiff for this change).

rerolled for 8.4.0

I think the issue is more global than just taxonomy.
I have nowhere this is not working.
I have to do as recommended here:
https://drupal.stackexchange.com/questions/205758/how-do-i-override-the-...

reroll for 8.5. Sorry, I lost track of this ticket.

Reroll of #84 for 8.6

This is still an issue in 8.7.4; the patch in #86 results in

Fatal error: Class 'Drupal\views\Plugin\views\argument\EntityArgument' not found in /Users/chris/Sites/devdesktop/tomorrowscities/web/core/modules/node/src/Plugin/views/argument/Nid.php on line 12

Updated title, as this applies to all entity references (not just taxonomy).

#87: The problem with #86 is that it's missing all of the new files so it's a bad patch. I'm not sure which would be easier:

• Rerolling #84 for 9.1 or
• adding the missing files to #86 and then rerolling that.

I'd be inclined to start with #84 because it seems more trustworthy.

Related: I postponed #2912332: Allow entity reference views arguments to display the entity label in titles and summaries on this one.

Rerolled patch #84

#93: Thanks for working on this! Which patch are you working from?

If possible, please provide interdiffs. I know this may not always be possible due to different baselines.

@colan worked on #84 and interdiff is not possible due to different core versions.

There is a workaround in #7.

Another way to work around the problem might be using preprocess hooks. For example, to set the node title in a views block on a node page. I guess, for taxonomy it would be similar.

/**
 * Implements hook_preprocess_HOOK() for block templates.
 */
function focaer_preprocess_{BLOCK-ID}(&$variables) {
  if ($node = \Drupal::routeMatch()->getParameter('node')) {
    $variables['label'] = 'Artikel von ' . $node->label();
  }
}

Not sure why ViewExecutableTest::testArgumentValidatorValueOverride is failing

Tested with Drupal 9.1.10:

Patch #93: working
Patch #101: not working

Has anybody been able to successfully get this patch working on 8.9? I've tried to re-roll it myself, and it does apply, but I keep getting the following error when I try to view a page with a contextual filter: Drupal\Component\Plugin\Exception\PluginNotFoundException: The "" entity type does not exist. in Drupal\Core\Entity\EntityTypeManager->getDefinition() (line 150 of /var/www/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php). This is a really frustrating bug that I need to fix but we're not quite ready for D9 yet. I haven't had luck with any workarounds.

Has anyone successfully tested this on 9.2? The latest patches do not apply, but even after I manually update the files that failed to patch and then run update.php, this is still not working for me. My title arguments ( arguments.field_tags_target_id ) in views are still rendered as ids.

A sample workaround for this until it gets fixed might look like this. In this example, I want to display the tag name in a text area if my view shows no results. Since views is rendering the tag id instead of the name like it's supposed to, I have to load the term name and replace the entire token with it. This is pretty sloppy, but it works. Hopefully this bug can be fixed. Currently, any views designed this way in 7 will not work in 8/9.

function MY_MODULE_views_pre_render($view) {
  if ($view->id() == 'VIEW_NAME' && $view->current_display == 'DISPLAY_NAME' && $view->empty['area_text_custom'] && $view->args[0] > 0) {
    $term = Drupal\taxonomy\Entity\Term::load($view->args[0]);
    $term_name = $term->get('name')->value;
    $content = &$view->empty['area_text_custom']->options['content'];
    $content = str_replace('{{ arguments.field_tags_target_id }}', $term_name, $content);
  }
}

Straight re-roll since this no longer applied to 9.3.x. Haven't reviewed yet.

9.2.x version for manual testing.

Another re-roll.

Patch from #110 does not apply anymore on 9.3.2.

@catch @raman.b could you guys explain why you started using $this->definition['entity_type'] as entity type instead of $this->definition['target_entity_type_id']? Looks like target_entity_type_id is the actual referred entity type while entity_type is the base entity type, and till #93 target_entity_type_id was used.

Rerolled the patch in #110, thanks!

In view of my notes above here's a re-roll of Berdir's patch #69.

Probably the issue is that there are two type of arguments: entity ID (like "nid") and referred entity ID (like e.g. "field_term_target_id"). The initial patches tried to handle the second case, then patches tried to handle the first case. Looks like we should have handle these two cases separately using two different argument types. Here's an attempt to do that.

9.3.x in 123 working for me. Thanks!

I can also confirm that the patch in #123 does finally work as well.

This may or may not be right on the spot, but at least related.
I'm on Drupal 9/Commerce Kickstart 3 installed in Romanian only.
I created a view showing product variations only (entities, not nodes) that bear all the same tag. It's a page with the address /taxonomy/term/%
I got inspired by this comment and came up with this no-code solution :
-Added a hidden field with the tag rendered as label and link to the product variation.
-Then, in Relationships, I added the relationship between tag name and tag ID. Marked as required.
-In the contextual filter I enabled the use of the relationship. This triggered the availability of {{ arguments.tid }} for me to use for overwriting the page title, while using tag ID as validation criterion.
And voilà, the tag name instead of tag ID appears in the page title.
I just hope that sharing with you my perspective as a user, not developer, helps.

The most recent patch applies but does not work for me on 9.4.5

I confirm that the patch #122 applies correctly and fix the issue in Drupal Core 9.4.5.

I tested it in a view block that requires to display the author name in its title. The view has a contextual filter of the author field and it is setup to use the token to resolve from the content id to the node title as shown in the attached screenshot

Thanks @alex-bukach

The issue summary here is simply a statement of the problem. The patch is > 40K and adds schema and a post_update function so let's have a description of the changes in the IS. I see an after screenshot in #129 so it seems that before and after screenshots should be added to the Issue Summary as well. I have added the standard template so it can be completed.

comment #128 states that the patch does not work on Drupal 9.4.5 so it does not seem ready for an RTBC.

Patch 122 didn't apply correctly, I have re rolled the patch for 9.5.x

@ameymudras I did not check your patch thoroughly, but the last lines in the patch definitely should be inside a loop (like in #122) while they are outside in your patch. Could you please check it and attach interdiff?

Fixed custom command failed error. please review

@Anchal_gupta both yours (#135) and @ameymudras (#133) patches contain issues I pointed out in #134. Could you please fix them and attach an interdiff between your patch and #123?

Thanks @Alex, I have fixed the issue and fixed phpcs issues

Patch in #122 worked for me on 9.4.7. No manual steps required, just applied the patch.

Before:

After:

The patch in 137 works for me

Added reroll of patch #137 on Drupal 10.1.x.

Try to fix cs errors.

Trying to fix the above issue

My working branch was out of sync which caused the above error, trying to fix the same

Fixing phpcs issue with the above patch

Tried to address tests of patch #146.

#148 did not apply with drupal 10.0.9 and PHP 8.1

This should work for 10.x

#151 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.

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 #2765297: Return translated term name on views "Content: Has taxonomy term ID (with depth)". 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 #3441844: Set budgets rather than exact numbers of asset size assertions. 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 #2765297: Return translated term name on views "Content: Has taxonomy term ID (with depth)" (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, #3317378: [random test failure] Drupal\Tests\media_library\FunctionalJavascript\WidgetViewsTest::testWidgetViews random fail 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

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 #3442227: Use labels in Views argument summaries for entity references 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?