Drupal provides an API function for scanning for tokens, so we should use it, in my opinion. This avoids edge-cases where this module assumes tokens are being used, but Drupal does not actually recognize them as tokens.

Patch is easy to roll, but would like some approval first.

CommentFileSizeAuthor
#9 2818873-9.patch3.01 KBrasikap
#3 2818873-3.patch4.11 KBrasikap

Comments

tstoeckler created an issue. See original summary.

rasikap’s picture

rasikap commented
Assigned: Unassigned » rasikap
rasikap’s picture

rasikap commented
Assigned: rasikap » Unassigned
Status: Active » Needs review
StatusFileSize
new 2818873-3.patch4.11 KB

Replaced strpos with token_scan. Please find the attached patch.

Status: Needs review » Needs work

The last submitted patch, 3: 2818873-3.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

Looks good to me. Thanks for the patch!

Not sure if we should change the existing update function, though. I think that could lead to confusing results if people have already run them, vs. people running the new code.

rasikap’s picture

rasikap commented

Thanks for the review tstoeckler.

So do I need to change anything in the above patch?

Regards,
Rasika

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

I think it would make sense to revert the changes in the *.install file, and just leave that file alone entirely. Would be good to get more opinions, though, on this.

rasikap’s picture

rasikap commented
Assigned: Unassigned » rasikap
rasikap’s picture

rasikap commented
Status: Needs work » Needs review
StatusFileSize
new 2818873-9.patch3.01 KB

Updated the patch to remove the changes in install file

Status: Needs review » Needs work

The last submitted patch, 9: 2818873-9.patch, failed testing.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Reviewed & tested by the community

There seems to be an issue with the testing of this module in general right now. The patch looks good, though.