Problem/Motivation

When creating a glossary view for taxonomy terms, the character "0" (zero) is replaced with "Uncategorized".
This comes from the value of the "empty field name" for the argument handler of taxonomy_term_field_data tid.
\Drupal\views\Plugin\views\argument\ArgumentPluginBase::summaryName() uses a empty check which returns false for 0, so the value above from the definition is used.
This doesn't happen for nodes as it doesn't have this value filled in for the related argument handler.

Steps to reproduce

  1. Create a view to list taxonomy terms;
  2. Add and configure an attachment to show as glossary, limiting the labels to the first character;
  3. Create a taxonomy term that starts with "0" (character zero);
  4. Visit the page. The label "Uncategorized" will be shown.

Proposed resolution

Add a !is_numeric() check to leave out string 0.

Remaining tasks

Tests and fix will be attached.

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#12 interdiff_10-12.txt806 bytesRatan Priya
#12 3291520-12.patch11.13 KBRatan Priya
#4 3291520-4-TEST-ONLY-FAIL.patch10.34 KBsardara
#4 3291520-4.patch11.06 KBsardara
#5 3291520-5-TEST-ONLY-FAIL.patch10.34 KBsardara

Issue fork drupal-3291520

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

sardara created an issue. See original summary.

sardara opened merge request !2415

cilefen’s picture

cilefen commented
Status: Active » Needs review
sardara’s picture

sardara commented
StatusFileSize
new 3291520-4-TEST-ONLY-FAIL.patch10.34 KB
new 3291520-4.patch11.06 KB

Adding the same content of the merge request as patch for external reference. Also attaching the separate test-only patch as I failed to have the separate commit of the MR tested alone.

sardara’s picture

sardara commented
StatusFileSize
new 3291520-5-TEST-ONLY-FAIL.patch10.34 KB

Messed up the test only patch.

Status: Needs review » Needs work

The last submitted patch, 5: 3291520-5-TEST-ONLY-FAIL.patch, failed testing. View results

sardara’s picture

sardara commented
Status: Needs work » Needs review

The test patch failed as expected.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community

Great work here, good expansion of the the test coverage so that it now actually tests the glossary!

The last submitted patch, 4: 3291520-4.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/views/src/Plugin/views/argument/ArgumentPluginBase.php
@@ -955,7 +955,7 @@ public function summaryArgument($data) {
     $value = $data->{$this->name_alias};
-    if (empty($value) && !empty($this->definition['empty field name'])) {
+    if (empty($value) && !is_numeric($value) && !empty($this->definition['empty field name'])) {

I think this should be:

$value = (string) $data->{$this->name_alias};
if ($value === '' && isset($this->definition['empty field name'])) {

We have the same empty problem with the empty field name if you have 0 as the empty field name :) - I think isset-ness is fine here. The empty check here is odd. Mostly this is a TranslatableMarkup object and the empty check will not work the same at all.

Ratan Priya’s picture

Ratan Priya commented
Assigned: Unassigned » Ratan Priya
Ratan Priya’s picture

Ratan Priya commented
Assigned: Ratan Priya » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3291520-12.patch11.13 KB
new interdiff_10-12.txt806 bytes

@alexpott,

I made the changes you required at comments #10

Needs review.

andras_szilagyi’s picture

andras_szilagyi commented

Test looks good, and the fix works as expected.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Bug Smash Initiative

Verified the issue exists without the patch
Verified patch applies to 9.5.x cleanly
Verified patch resolved the issue described in the issue summary.
See there is a tests-only patch in #4
Reviewing the code looks good.

Moving to RTBC

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.4.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

I've run the tests locally on 10.1.x and it works great. This is a nice fix to views for an odd use-case. We definitely need to learn to be better at handling strings and empty() checks.

As this is a bug fix and does not involve API changes backporting to 9.5.x

  • alexpott committed 5496d64 on 10.1.x 
    Issue #3291520 by sardara, Ratan Priya, alexpott, smustgrave: Incorrect...

  • alexpott committed 50e24a2 on 10.0.x 
    Issue #3291520 by sardara, Ratan Priya, alexpott, smustgrave: Incorrect...

  • alexpott committed 2277e2e on 9.5.x 
    Issue #3291520 by sardara, Ratan Priya, alexpott, smustgrave: Incorrect...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: 9.5.x-dev » 9.4.x-dev

Discussed with @longwave - we agreed to backport to 9.4.x

Committed f4f5725 and pushed to 9.4.x. Thanks!

  • alexpott committed f4f5725 on 9.4.x 
    Issue #3291520 by sardara, Ratan Priya, alexpott, smustgrave: Incorrect...

Status: Fixed » Closed (fixed)

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