Problem/Motivation

There is an option to set #dialog to FALSE when calling theme('token_tree') or theme('token_tree_link'). By default, it is TRUE and all code within token module sets it to TRUE anyway. Setting it to FALSE throws an error and fixing the error does not change anything (except change the look of the dialog slightly). In other words, there is a dialog no matter the value of #dialog.

Proposed resolution

This brings up a question? What is the point of the variable? Is it an outdated functionality we no longer use? If yes, we should remove it. If not, we should fix it to whatever behaviour.

Remaining tasks

- Remove the dialog variable.
OR
- Fix it to whatever behaviour we need.

User interface changes

None

API changes

Possibly removing the dialog variable.

Data model changes

None

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

I am happy to write the patch for either of the scenarios. I just wanted to know from the maintainers what is the behavior intended.

Berdir’s picture

I'm confused. What you are saying here seems to contradict #2640138: Use token_tree_link instead of token_tree to render links, where you remove the option, which is the same as setting it to FALSE? I tried, it works fine for me.

hussainweb’s picture

I think the IS was a little confusing. If you set #dialog to FALSE for theme('token_tree'), it will just render the entire token tree, which is fine. But if you set #dialog to FALSE for theme('token_tree_link'), then you would see an error 'Unsupported operand types'. That error is just a small one-line fix and the code moves ahead. However, you will still get the same link and a similar dialog which works but doesn't look the same (due to missing libraries).

And I am not really removing the #dialog option from theme_token_tree_link in #2640138: Use token_tree_link instead of token_tree to render links. I am just removing it from theme_token_tree where the option doesn't make sense, IMHO. I intend to remove the #dialog option here, if we are not using it. The other issue is orthogonal to this, not contradictory.

Berdir’s picture

Ah, yes. lets remove #dialog from token_tree_link.

hussainweb’s picture

Status: Active » Needs review
FileSize
3.38 KB

Attaching a patch which removes #dialog. As always, I tested it manually to see that the link and dialog is still working on different pages.

Status: Needs review » Needs work

The last submitted patch, 6: dialog_option_for_token-2640170-6.patch, failed testing.

hussainweb’s picture

Ah, I created the patch over the changes in #2640138: Use token_tree_link instead of token_tree to render links. If that is not being committed soon, I will create a new patch here.

Berdir’s picture

Status: Needs work » Needs review
hussainweb’s picture

Great! All passed. Thank you for committing the other patch. :)

Berdir’s picture

Status: Needs review » Needs work
+++ b/token.pages.inc
@@ -39,7 +37,7 @@ function theme_token_tree_link($variables) {
   // Set the token tree to open in a separate window.
-  $variables['options']['attributes'] += array(
+  $variables['options']['attributes'] = array(
     'data-dialog-type' => 'dialog',
     'data-dialog-options' => json_encode(array(

this change seems wrong, that overwrites the token-dialog class that you set above?

hussainweb’s picture

Sorry, I had done that for testing a related issue. I'll check and revert that.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
551 bytes
2.96 KB

Fixing for the issue in #11.

Berdir’s picture

+++ b/token.pages.inc
@@ -11,19 +11,17 @@ use Drupal\Core\Url;
 /**
- * Theme a link to a token tree either as a regular link or a dialog.
+ * Theme a link to a token tree either as a regular link.
  */

That description doesn't make sense. It's the regular link part that we're removing and it still says either but only hase one case?

"Theme a link to a token tree shown as a dialog." ?

hussainweb’s picture

Yes, I didn't realize it. I changed it as per your suggestion. I skipped the interdiff (that's the only change). I hope that's okay.

Thanks for the reviews. :)

Berdir’s picture

Status: Needs review » Fixed

Thanks, committed.

  • Berdir committed 76cc428 on 8.x-1.x authored by hussainweb
    Issue #2640170 by hussainweb: Dialog option for token tree link does not...

Status: Fixed » Closed (fixed)

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