Problem/Motivation

The body of hook_tokens() was commented out early in the porting process and hasn't been updated since.

Proposed resolution

Uncomment and update hook_tokens().

Remaining tasks

Create patch.
Create test for flag token functionality.

User interface changes

None.

API changes

Flag tokens would work again.

CommentFileSizeAuthor
#43 flag_hook-tokens_2500091-43.patch30.81 KBartem_sylchuk
#38 interdiff-2500091-33-38.txt6.09 KBmartin107
#38 2500091-38.patch30.72 KBmartin107
#33 interdiff-2500091-30-33.txt23.2 KBmartin107
#33 2500091-33.patch29.75 KBmartin107
#30 interdiff-2500091-26-30.txt1.19 KBpartdigital
#30 2500091-30.patch10.96 KBpartdigital
#26 interdiff-2500091-22-26.txt1.63 KBmartin107
#26 2500091-26.patch10.91 KBmartin107
#22 2500091-22.patch9.28 KBlegolasbo
#22 interdiff-21-22.txt352 byteslegolasbo
#21 interdiff-16-21.txt2.14 KBlegolasbo
#21 2500091-25.patch9.24 KBlegolasbo
#16 2500091-16.patch9.04 KBlegolasbo
#11 interdiff-2500091-10-11.txt3.11 KBmartin107
#11 2500091-11.patch8.86 KBmartin107
#10 2500091-10.patch7.18 KBlegolasbo
#7 re_implement_hook_tokens-2500091-7.patch4.94 KBAnonymous (not verified)
#4 re_implement_hook_tokens-2500091-4.patch4.55 KBAnonymous (not verified)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Assigned: Unassigned »
Status: Active » Needs work
joachim’s picture

Status: Needs work » Needs review

Thanks for taking this on!

Though note that 'needs work' status means that there is a patch which needs further work. If you mean that the issue is being worked on, that's just 'active'. See https://www.drupal.org/node/156119.

Anonymous’s picture

Status: Needs review » Active

Sorry, thx for explanation)

Anonymous’s picture

Status: Active » Needs work
FileSize
4.55 KB

Here is the patch where I've started to re-implement this hook.
I heed some advice for action tokens replacement.
Also I can't find flag_create_link definition in d8 version.

Anonymous’s picture

Status: Needs work » Needs review
socketwench’s picture

There isn't a flag_create_link() function in D8. Instead, you need to get the link type plugin and call its renderLink() method. Look at flag_entity_view() for ideas.

Unfortunately, renderLink() requires you to do all the access checks yourself, so we may need to break that off into another issue to fix.

Anonymous’s picture

Assigned: » Unassigned
FileSize
4.94 KB
joachim’s picture

> Unfortunately, renderLink() requires you to do all the access checks yourself, so we may need to break that off into another issue to fix.

There's an issue for that IIRC.

socketwench’s picture

Status: Needs review » Postponed
legolasbo’s picture

Status: Postponed » Needs review
FileSize
7.18 KB

#2488856: add a wrapper around buildLink() that checks access has been fixed so time to reopen this.

I've rerolled the patch and adjusted it to match the changes in both flag as well as core.

martin107’s picture

A couple of minor things

there was a case of array() -- when the D8 coding standard says [] is prefered.

I have injected the token service into ActionLinkTypeBase

There is a question of balance ...

we have not historically had tests in this area.

but here there is potentially raw text from the public internet being displayed on a browser - without twig sanitisation.

so I think some testing is required... although I don't want to go overboard as we could easily quadruple the size of the patch... anyway something I am going to sleep on before suggesting something considered.

TR’s picture

Marked #2906588: Removed deprecated method/Class check_url from code base as a duplicate, but it only fixed one line in hook_tokens().

martin107’s picture

So doing some background reading ...

from this change record https://www.drupal.org/node/2578365

hook_tokens() implementors are no longer responsible for "sanitization". If the token value is plain text, just return the string.

So that simplifies things....

What complicated things is making sure we take care of this change record https://www.drupal.org/node/2528662

titled "Token replacement needs to consider cacheability metadata"

But that is ok the CR has laid on some examples.

socketwench’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

So far so good! Needs tests, of course.

@martin: Do we need a new patch to remove the sanitization stuff?

martin107’s picture

Do we need a new patch to remove the sanitization stuff?

Yes, that is part of it... I will see what I can fit in over the holiday period.

legolasbo’s picture

martin107’s picture

as pointed out by bedir

https://www.drupal.org/project/flag/issues/2939121#comment-12449244

D8.6.x is going through growing pains .. services have become private and that is causing error.
D8.6.x will fix itself through a critical.

In short I am retesting on 8.5.x

bogdog400’s picture

I've been trying to use the Flag module with the Token module with Drupal 8.5 but the token blocks aren't being replaced.

I've put this in the Flag text field:

([node:flag-approve2-count] users have approved2 this) [node:flag-bookmark][node:title]

All three of the square bracket delineated blocks aren't changed. They appear on the page.

The comments here suggest that this feature isn't implemented yet in the module for Drupal 8. I thought I would check to see if I'm barking up the right tree. If anyone has any suggestions, I would love to hear them.

Thanks,

TR’s picture

@bogdog400: Token support has not been ported to D8 yet - that's what this issue is all about.

You can help get this finished by using the patch in #16 on your site, then posting here about your experiences with it - did it fix your problem?

bogdog400’s picture

Thanks for the quick response. I'm happy to test. I'm just running into trouble installing the patch on a linux box, no doubt because I just don't understand the patch command well enough.

Here's the error I get:

|diff --git a/src/ActionLink/ActionLinkTypeBase.php b/src/ActionLink/ActionLinkTypeBase.php
|index a14e37e..be013fa 100644
|--- a/src/ActionLink/ActionLinkTypeBase.php
|+++ b/src/ActionLink/ActionLinkTypeBase.php
--------------------------
File to patch:

Obviously I'm not executing it in the right directory or with the right options. If you could spell it out for me, I'm happy to apply and report back.

legolasbo’s picture

I adjusted the patch a little because it was causing issues when menu items were routed trough the token system. Also cleaned up the code a little while I was at it.

legolasbo’s picture

socketwench’s picture

I like this patch! I'm still perplexed as to why the tests are failing so hard, though.

TR’s picture

Tests fail because the constructor for ActionLinkTypeBase was changed, but the constructor for AJAXActionLink (which is a subclass of ActionLinkTypeBase and which invokes parent::_construct()) wasn't changed by the patch.

martin107’s picture

Assigned: Unassigned » martin107

Tests fail because the constructor for ActionLinkTypeBase was changed,

I have some time now ... so I will get the patch over that particular hump.

martin107’s picture

socketwench’s picture

Tests fail because the constructor for ActionLinkTypeBase was changed...

*facepalms* Yeah, that'd do it.

The test failure from #26 looked transient, and disappeared when rerunning the tests.

I'd still like at least one test for this before committing it.

martin107’s picture

Assigned: martin107 » Unassigned
muaz91’s picture

Hi,

I tried to pull out user id from flag using the [flag-action:entity-id], but it does not work. It came out empty.

I am using the patch from #26 and tested out with business rules module.

partdigital’s picture

FileSize
10.96 KB
1.19 KB

I was able to apply the patch successfully however, I was getting fatal errors with the flagging token. I've updated the patch with some minor changes and it seems to be working now for my context. The changes I made involve passing the flagging date.

In summary, in flag.tokens.inc I removed the deprecated date_format function and changed this...

$flagging->get('created')

To this

$flagging->get('created')->value
Prodigy’s picture

Hey @partdigital -- what version did you make those changes off of?

ALSO, IS THIS ESSENTIALLY WHAT EVERYONE ELSE IS GOING THROUGH??

1.) SCREENSHOT OF FLAG CREATE PAGE SHOWING WHERE YOU MUST SELECT A TOKEN OR THE PAGE WILL NOT SAVE. THE PROBLEM HERE IS NO MATTER WHICH TOKEN YOU SELECT, IT PRINTS THE ACTUAL TOKEN VERBATIM (ex [node-author] INSTEAD OF WHAT YOU ARE EXPECTING (example 'Admin'). SCREENSHOT BELOW SHOWS STEP 1. UNTIL THIS IS FIXED, THIS MODULE IS D8 UNUSABLE.

Flag create page showing the REQUIRED token. The easy fix here is to remove the use of tokens until someone can fix the use of them. Either is fine. I'm happy to sponsor.
https://imgur.com/tF5V5Qi

2.) Steps to recreate this are very easy. Edit the default flag and change the token or keep it the same, either way you will see the unprocessed token text, instead of the processed token. See below (this is the actual node).
https://imgur.com/rygFMNw

3.) Lastly, just like above...but this is a view)
https://imgur.com/XaVHHcZ

What about just removing the use of tokens until we get this fixed? Or I'll sponsor the fix. Would not be my first sponsor to this module, only prob then is it was called 'Views Bookmarks'

martin107’s picture

Assigned: Unassigned » martin107
Status: Needs review » Needs work

I have made some progress on this

I normally err on the side of posting early .. but what I have is just too fragmented -- and I am still obviously working the form of things out ..

I am assigning this to me. Just to signal that I have some time .. this is a priority and it is going to get some love form me in the immediate future.

martin107’s picture

FileSize
29.75 KB
23.2 KB

This is just an update .. I am stil working away.

A) The only signficant design decision I made needs explaining as it introduces
a breaking change that will break code using this beta in production
... but this is still beta.

It comes from the observation that in relation to strings stored in the Flag entity:
there are two separate code paths/activites.

(Path 1) When handling form... flag_form_alter() and FlagFormBase :-

Access to the unproceed/raw token laiden text eg. "this is my [flag:name]" is common.

The change is that these are now accessed via $flag->get('long_text'), etc

(Path 2) Everywhere else the processed/converted text is needed, coincidently that is also everywhere the '$action'
input is required.

So here are the BC changes

FlagInterface
  public function getShortText($action, array $data = []) : string
  public function getLongText($action, array $data) : string;
  public function getMessage($action, array $data = []) : string;

in short a $data 'contextual' parameter is needed to provide the action, flagging or node
for the tokenizer to function.

I am also enforcing that the output of the functions is a string.
This is only justified -- 'cos other breaking changes are occurring.

B) I have a first cut version of the tests... see FlagTokenTest.php

It is a good first start- I intend to make it more complete... but I am 'posting early, post often'

A small number of Functional/Integation tests are planned.

C) Writing the tests has allowed me to pick out a few bugs and get things working.
see flag.tokens.inc

D) I have simplified a $langcode isset()/if statement in a few instances.


The null coalescing operator (??) has been added as
syntactic sugar for the common case of needing to use a
ternary in conjunction with isset().

https://www.php.net/manual/en/migration70.new-features.php

Work still todo

A) Implement the new theme() properly. See the add form.
B) A mini security review .. it occurs to me that maybe a rouge contrib module could use the flag module to
inject a entity into the method now using contextual data and thereby gain access to priveleged fields.
That 'maybe' needs careful consideration.
C) tests enhancement.
D) I need to think more about our use of BubbleableMetadata

TR’s picture

The null coalescing operator (??) has been added as syntactic sugar

Note this will make Flag incompatible with Drupal 8.7, which is still a supported version of core. Currently, more than 50% of all D8 sites still use Drupal core < 8.8.0. Likewise for the return value typing in public function getShortText($action, array $data = []) : string - this is only possible with core 8.8.0 and greater. In principle these changes can only be made in June after core 8.9.0 is released and core 8.7 becomes unsupported.

The only significant design decision I made needs explaining as it introduces a breaking change that will break code using this beta in production ... but this is still beta.

Policy for Drupal core is to provide beta-to-beta upgrade paths, and that is also the expectation for contributed modules. Otherwise this isn't really a beta, it's just a mis-named alpha. So while an API breaking change is not forbidden, a hook_update_N() or a BC layer should be provided and a @deprecation should be added to the "old" API function to help users transition.

Berdir’s picture

Note this will make Flag incompatible with Drupal 8.7, which is still a supported version of core. Currently, more than 50% of all D8 sites still use Drupal core < 8.8.0. Likewise for the return value typing in public function getShortText($action, array $data = []) : string - this is only possible with core 8.8.0 and greater. In principle these changes can only be made in June after core 8.9.0 is released and core 8.7 becomes unsupported.

It makes it incompatible with PHP5, which Drupal 8.7 still halfway supports. Not the same thing. Modules are free to require whatever PHP version they want, that's why the ability to specify that in the info.yml exists. Whether or not it is worth to break PHP5 support for some syntactic sugar and bother with having to update the .info.yml for that, that's another question.

Also, there is also no rule that forces any module to support 8.7 as long as it has security support. Again, there are new and improved ways to specify which drupal core a module supports for a reason. 8.7 is in security-support, so that sites have a timeframe to update from 8.7 to 8.8, nobody is forced to update to latest flag version if they need to stay longer on 8.7 than others. I'll start requiring 8.8 soon in many of the modules that I maintain.

I do agree on the API changes, breaking the interface should be avoided and if at all possible, we should find a different way to achieve the goal here. Which I'm not sure I understand yet, didn't review the patch yet. But for starters, the whole $sanitize concept simply doesn't exist anymore, that option is gone, compare with e.g. node_tokens() in 7.x and 8.x.

martin107’s picture

TR, Berdir .. thanks for you comments, and I will conform. [ thanks for the detailed explanation ]

I anticipate a few more iterations before I consider my work to be polished ... I will undo those changes as the last step.. as I want to benefit for those warnings while I work.

TR’s picture

It makes it incompatible with PHP5, which Drupal 8.7 still halfway supports. Not the same thing.

Correct.

In my experience, there's inertia with most sites. You could even say a LOT of inertia. How many sites make major changes every 6 months? Do we want Drupal to be a product that is only usable by large organizations with dedicated staff to maintain a site? Anyone who is still on <=8.7 is much more likely than most to have a site that is using PHP 5, because they probably started the site in an earlier version of D8 (or even D7) where the most recent versions of PHP weren't available. And anyone who is using an older version is probably less able to deal with breaking changes like the ones I mentioned.

Whether or not it is worth to break PHP5 support for some syntactic sugar and bother with having to update the .info.yml for that, that's another question.

If it takes no effort to support all currently-supported configurations of core, then that is what should be done. I don't feel that this is optional. People make plans based on what Drupal promises - Drupal has promised support for D8.7, and promised support for PHP 5 in D8.7, so that is what we should all strive for. Drupal's promises are so minimal that it's the least we can do to try to fulfill them. Would you buy a car that was only guaranteed to work for 6 months? A web site is a large investment of time and money, so providing the expected 1 year of support for a minor version is the LEAST we can do.

Modules are free to require whatever PHP version they want, that's why the ability to specify that in the info.yml exists.

True, but that should be a conscious decision based on knowledge of the facts and as a last resort, not simply because it's more convenient and cooler for a developer to use the latest syntax rather than support a version of Drupal that isn't even 1 year old at this point. In the real world, long-term support matters, and breaking things every 6 months for no good reason is not very user friendly.

Contributed modules should conform to Drupal requirements as much as possible, otherwise we will have a situation where modules won't work with each other even though they are all technically written for the same version of Drupal. We had this to some extent in D7 with e.g. jQuery Update, where some modules were written for the default Drupal version of jQuery but some other modules required the latest version of jQuery and since newer versions of jQuery weren't completely backwards compatible with previous versions, we ended up with incompatible ecosystems of modules.

Also, there is also no rule that forces any module to support 8.7 as long as it has security support.

Of course not, but reverse that and the same holds true - why should you force sites to use only the most recent < 3 month old version of Drupal? We all know that bugs are most likely encountered in new releases, and as releases mature they become more stable because bugs are found and fixed. By coding for HEAD and not considering BC, you are taking a developer-centric version of the web rather than user-centric. IMO there are only limited circumstances in which you can justify throwing away support for supported versions of core. In this case, since it is trivial to continue to support the supported core configurations, then that is what should be done. Breaking someone's perfectly good and working D8.7 install just because you want to use "syntactic sugar" is not OK.

It's a little disingenuous to imply that modules have no obligation to conform to Drupal core requirements guarantees. Some modules are used by a large fraction of the Drupal community, so it would be a case of the tail wagging the dog if each were free to define its own requirements without consideration of what core supports.

Drupal is now making BC-breaking changes between minor versions, and minor versions are only supported for 1 year at the most (including security support). This is already a very short period of time, so cutting that shorter out of laziness is just not acceptable IMO. If you want to see Drupal gain market share, and not continue to loose both market share and absolute number of users, an effort has to be made to support working installations for more than a few months. It's not hard - there's no reason to make the above breaking syntax changes as the old syntax will still work perfectly well.

I'll start requiring 8.8 soon in many of the modules that I maintain.

Well, contrib has been forced into a bad position by core when it comes to the D9 upgrade. For many non-trivial modules it's impossible to support both D8.7 and D9 because of the huge number of breaking changes made in D8.8. The deprecation of PHPUnit 4 in D8.8, for instance, means that some Unit tests written for D8.7 will no longer work in D9, and if they're re-written for D9 they will not work in D8.7. So contrib either has to be bullied into moving to D9 early and abandoning D8.7 or each contrib needs to write it's own BC layer for the features it needs (why aren't these layers included in core rather than forcing each contrib to write its own?)

If you really need to drop D8.7 support because there are things you just can't do in D8.7 that's one thing, but dropping support just because you want to be at the cutting edge of Drupal development is something else. I'm personally going to make that extra effort to continue to support all currently-supported core configurations. I don't care about unsupported configurations at all - I'm not going to try to continue to support D8.5 for instance - but users should be able to count on the promises that Drupal makes as to how long a release will be supported.

So to get back to this issue ...
My point was that null coalesce operators and return type declarations are unnecessary and break compatibility with *some* currently supported Drupal core configurations. Flag is in beta, and any users of this beta should be able to get bug fixes and new features without have to upgrade their entire site (which can be a big task, even for upgrades between minor versions of core. We all know these "minor" core version upgrades don't always go smoothly ...) Because there is nothing in Flag that requires this new syntax, it should be avoided.

martin107’s picture

This is just another update I am still working away

A) Regarding the reviews, I have unwound my null coalescing operator overstep.
More unwinding to follow.

B) I have added first cut test coverage for [node:flag-FLAG-ID-count]
FlagTokenTest::testEntityCountToken()

C) I ran a few other flags tests and fixed up my mistakes.

D) This history of people with differing views on the internet .. does not commonly have happy outcomes.

So before I say anything -- please understand I only want to further the exchange of ideas.

- and only want to extend understanding, and positivity.

The position above is well articulated and money/time issues are a harsh reality.

Maintenance costs for a website -- are the first things to be rejected by stakeholder's holding the purse strings.

So I say this with the battered memory of someone debriefing - searching
for constructive take-aways from a failure.

I belatedly came to this true-ism :-

"If there is no ongoing maintenance plan for your project -- ingest contrib projects with care."

Know if the issue queue is being well serviced. Is it sponsored?

I love the flag module, it is a fine tool and it is well serviced by its maintainer's.
Who are really generous with their time - and the advice is always at a 'consultant' grade level.

But there is a log jam regarding alpha/beta releases. [I will help out in whatever way I can to undo this ].

The uncomfortable truth if your project is normal and there is no maintenance planned for say the next 3 years
- then you should walk away from all alpha/beta releases in general.
- Under this discipline the flag module must be discounted from consideration.

Also if you can tell me how to avoid this trap please let me know... but it is a fundamental mistake.

I always want to be constructive, and am generally a person who "goes along, to get along".

So maybe rigid policies are not helpful - I am happy to take care and make adjustments for the actual installed base.

That is what I meant in my head by 'conform'. I am sorry I did not intend to be thoughtless.

Anyway in the coming days I hope you will be understanding if I push back with

"Well - all this is still pre-production so making changes is appropriate."
( with fallback, triggers and deprecation messages )

and I will try and fix your pain points.

martin107’s picture

I just want to check my logic around a little roadblock, so some advice would be welcome

Regarding my implementation of [flag-action:entity-url]

I think I maybe at the intersection of a few open core/rules related issues.
and I may not be able to process until other issues are resolved.

Specifically using the context system in relation to derived actions, I think I need the functionality provided only by a incomplete core issue.
#2011038: Use Context system for actions

Let me layout my problem in full

EntityFlagActionDeriver is the factory thing which

loops over all flags :-

then reaches into the flag and set the 'type' on the action with

    'type' => $flag->getFlaggableEntityTypeId(),
  

so far so good ... but to complete the flag-action:entity-url

I think I require access to the $entity object/parameter which at the moment is only available within the execute() method

FlagAction {
  public function execute($entity = NULL) {}
}

So my roadblock, is that I think I need that $entity object but within the flag_token() function.

I maybe missing something but within hook_token() I think I need something like

$entity = $action->getContextValue($entity);
return $entity->url();

Looking at the open core issue ... and the last patch

https://www.drupal.org/files/issues/2018-12-26/2011038-100.patch

essentially Action base is modified to include 'execution' abilities

ActionBase extends ExecutablePluginBase

So I could then get at the context $entity in our hook_token() .... I think that is true . MAYBE

Anyway, if this discussion trigger a better solution -- I would be grateful.

Prodigy’s picture

Thank you Martin!

martin107’s picture

Assigned: martin107 » Unassigned
Prodigy’s picture

Image Showing Current Token Behavior

Above is a screenshot for anyone who doesn't want to replicate or what have you...

Same behavior for all token.

I too am working on a fix and what Martin says makes most sense to me.

NOT having access to the entity within the $flag_token() function we likely need something like...

$entity = $action->getContextValue($entity);
return $entity->url();
artem_sylchuk’s picture

My project used this patch and it stopped applying after 4.x update.
So I did a quick re-roll.

I'm not quite sure what is going on within the patch code though.

AlfTheCat’s picture

Patch from #43 applies but it seems only to work for node flags. I'm trying to use a token in a flag link on a custom ECK entity and it's not working.