I'm storing some arbitrary info in the item "data" column with a custom widget.

I also want to display it in the markers for the map so the easiest way would be using tokens.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

rodrigoaguilera created an issue. See original summary.

rodrigoaguilera’s picture

Status: Active » Needs review
FileSize
1.76 KB

I created a patch with tokens for data that go up to two levels deep.

ChristianAdamski’s picture

Status: Needs review » Needs work

While I see the benefit of this, this patch is not acceptable. You're re-introducing deprecated code. Also, you are not checking, whether 'data' itself is traversable.

This "if (is_array($var) || ($var instanceof Traversable)) {" might do the trick.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
1.5 KB
1.1 KB

Thank you for the review.

Sorry I made the modifications in the stable release then I copied the whole file.
Added your fix too.

ChristianAdamski’s picture

Status: Needs review » Needs work

Hmm,

I'm still hesitant about this. I simply don't know well enough, how tokening things works in Drupal. In worst case, we could break a lot of peoples installation, because Drupal tries to tokenize things with unexpected results.

So we need tests for this, to check what happens when you store weired things in 'data' and Drupal has to manage.

The patch itself seems a good idea though, although you are still not checking whether 'data' is traversable. That is the "$metadata" variable in your code. You just assume you can call "foreach" on it.

TL;DR: this needs tests.

ChristianAdamski’s picture

Also, it might be a good idea to cast things to "string" before handing them over to token, and wrap things in try / catch, just in case.

rodrigoaguilera’s picture

Status: Needs work » Needs review
FileSize
3.32 KB
2.94 KB

I'm still hesitant about this. I simply don't know well enough, how tokening things works in Drupal. In worst case, we could break a lot of peoples installation, because Drupal tries to tokenize things with unexpected results.

AFAIK Adding a new token doesn't have much risk. The new token does not appear in people's current code. Tokenizing is just a replace and this token is not even global or represents an entity, it does the replacement only when is requested by this module without interfering with the global token system.

I think I addressed your concerns with this new patch. Not so sure about the try/catch

ChristianAdamski’s picture

Minor fixes added. If it goes green, I'll commit it.

ChristianAdamski’s picture

Status: Needs review » Fixed

Committed. Thanks!

  • rodrigoaguilera authored 04f0a5d on 8.x-1.x
    Issue #2892117 by rodrigoaguilera, ChristianAdamski: Add tokens for item...
rodrigoaguilera’s picture

Status: Fixed » Needs review

Thanks to you for such a thorough review!

Great module by the way and keep on doing an amazing maintenance :)

ChristianAdamski’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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