Problem/Motivation

Token descriptions and token type descriptions should only ever be used if titles cannot hold sufficient information. They're an optional addition, and should never be a requirement.

Proposed resolution

This is primarily a policy decision. Patch updates the the descriptions to list them as optional and removes the description on a few example descriptions eg. node title.

Remaining tasks

n/a

User interface changes

n/a

API changes

Change notice required

Original report by xano.

Descriptions should only ever be used if titles cannot hold sufficient information. They're an optional addition, and should never be a requirement.

CommentFileSizeAuthor
#11 drupal_1911768_11.patch3.2 KBXano
#1 drupal_1911768_00.patch2.94 KBXano
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Xano’s picture

Status: Active » Needs review
FileSize
2.94 KB
Xano’s picture

#1: drupal_1911768_00.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, drupal_1911768_00.patch, failed testing.

Xano’s picture

Status: Needs work » Needs review

#1: drupal_1911768_00.patch queued for re-testing.

Dave Reid’s picture

Let's just actually make them optional rather than providing defaults for them. It's pointless to merge in NULL values.

Xano’s picture

It prevents us from having to add isset() everywhere and gives us a behavior that is more similar to using classes, without actually having to convert the entire system.

Dave Reid’s picture

That's not a pattern that is used in Drupal core's info hooks at all for truly optional values though. I don't think we should start here.

Xano’s picture

It does for annotations, for instance. We are clearly moving to a situation in which we provide default values for optional information.

Xano’s picture

Assigned: Xano » Unassigned
Xano’s picture

Bump.

Xano’s picture

Issue summary: View changes
FileSize
3.2 KB

I removed some redundant node token descriptions as a proof of concept.

Xano’s picture

11: drupal_1911768_11.patch queued for re-testing.

jsibley’s picture

Issue 2159989 for Drupal 7 was pointed to this issue. Yet, as pointed out in that issue, this appears to be for Drupal 8.

Is this fix supposed to address issues in Drupal 7, as well, once Drupal 8 is fixed. Will it be backported and will that status be reported as part of this issue?

Thanks.

Xano’s picture

Maybe, depending on how much it would break backwards compatibility. That discussion will not be started until this has first been fixed for Drupal 8, though.

Xano’s picture

1: drupal_1911768_00.patch queued for re-testing.

The last submitted patch, 1: drupal_1911768_00.patch, failed testing.

Xano’s picture

11: drupal_1911768_11.patch queued for re-testing.

heddn’s picture

Issue summary: View changes
heddn’s picture

Issue tags: +Needs backport to D7
heddn’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the code. It makes sense. There seems to be general consensus on the recommended approach. Marking RTBC.

Dave Reid’s picture

Status: Reviewed & tested by the community » Needs work

The patch in #11 doesn't actually include the merging of defaults that was in patch #1.

Dave Reid’s picture

Status: Needs work » Reviewed & tested by the community

Ok, confirmed with @Xano today that not merging in default is actually the desired behavior, so confirmed this is RTBC. I'm glad we're getting rid of the completely unhelpful descriptions that are just duplicates of the token name itself. Should we have a follow-up issue to review all the existing tokens for redundant descriptions?

I don't think this should be backported to Drupal 7 however.

  • Commit f9dc1f1 on 8.x by alexpott:
    Issue #1911768 by Xano: Added Make token and token type descriptions...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs backport to D7

Committed f9dc1f1 and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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