Problem/Motivation

There are various form_alters in the token.module to add tree links or other processing for various forms. Some of these alters have been carried over from Drupal 7 and those form id's may have changed. We should either fix them or remove them if they are not valid anymore.

List of form alters

  • token_form_alter - Adds a submit handler to forms which could affect the tokens available on the site. It affects forms 'profile_field_form', 'profile_field_delete', 'system_add_date_format_type_form', 'system_delete_date_format_type_form'.
  • token_form_block_form_alter - Adds a label and validation for tokens.
  • token_field_widget_form_alter - Process tokens in field descriptions.
  • token_form_field_config_edit_form_alter - Adds tree link to field config form.
  • token_form_system_actions_configure_alter - Alters the configure action form to add token context validation and adds the token tree for a better token UI and selection.
  • token_form_user_admin_settings_alter - Alters the user e-mail fields to add token context validation and adds the token tree for a better token UI and selection.

Proposed resolution

Determine if the alters are necessary, fix or remove them.

Remaining tasks

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#17 fix_or_remove_unused_form_alter-2641226-17.patch7.21 KBhussainweb
#17 interdiff-15-17.txt506 byteshussainweb
#15 fix_or_remove_unused_form_alter-2641226-15.patch7.18 KBhussainweb
#15 interdiff-8-15.txt4.23 KBhussainweb
#8 fix_or_remove_unused_form_alter-2641226-8.patch3.05 KBhussainweb
#8 interdiff-5-8.txt1.27 KBhussainweb
#5 fix_or_remove_unused_form_alter-2641226-5.patch2.43 KBhussainweb
#2 fix_or_remove_unused_form_alter-2641226-2.patch2.65 KBhussainweb
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hussainweb created an issue. See original summary.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Active » Needs review
FileSize
fix_or_remove_unused_form_alter-2641226-2.patch2.65 KB

The patch removes form alters for:

  • profile_field_form
  • profile_field_delete

It fixes form id's for others. I also verified each form change manually.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented 
+++ b/token.module
@@ -153,16 +151,6 @@ function token_form_block_form_alter(&$form, FormStateInterface $form_state) {
- * Implements hook_widget_form_alter().
- */
-function token_field_widget_form_alter(&$element, FormStateInterface $form_state, $context) {
-  if (!empty($element['#description']) && is_string($element['#description'])) {
-    /* @var \Drupal\Core\Field\FieldItemListInterface $context['items'] */
-    $element['#description'] = Xss::filterAdmin(\Drupal::token()->replace($context['items']->getFieldDefinition()->getDescription()));
-  }
-}
-
-/**

For those reviewing, this alter is not necessary anymore. I verified that Drupal core replaces tokens even without this function. In fact, due to a check on is_string, it never actually does anything.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Category: Task » Bug report
Status: Needs review » Needs work
Issue tags: +Needs tests

Confirmed, token replacement for field descriptions is done in core.

However, I did find a small bug in a form alter function, lets fix this here too. token_form_user_admin_settings_alter() has the original string that it replaces (which makes sense, so it works for translated strings). But that string changed in core, so that doesn't work anymore.

Also tagging with needs tests. It should be relatively easy to write some very basic test coverage, visit that form and ensure that the link/text shows up.

  1. +++ b/token.module
@@ -115,15 +116,12 @@ function token_theme() {
     // User picture form.
     case 'user_admin_settings':
     // System date type form.

    The user picture option is gone from the UI, that's just a field now that exists or doesn't. So I think we can remove this. 

  2. +++ b/token.module
@@ -115,15 +116,12 @@ function token_theme() {
-    case 'system_add_date_format_type_form':
-    case 'system_delete_date_format_type_form':
+    case 'date_format_add_form':
+    case 'date_format_delete_form':

    Thinking about this, date formats are config entities now. And they might also be imported through config sync. Lets convert this to hook_date_format_insert()/delete() hooks.

    I think nothing will be left here anymore then.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
fix_or_remove_unused_form_alter-2641226-5.patch2.43 KB

Just doing a reroll so that we have something to refer to later.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs review » Needs work

Back to needs work for comments in #4.

There are already tests for this in TokenFieldUiTest. Do you want to test each and every form we alter?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

I think basic test coverage for those would be good. As the example with the user form shows, only a tiny change in the translatable string can break our functionality.

I'm OK if we don't add test coverage for e.g. the actions. And instead of working on tests for the date tokens and cache clearing in token.module, we could just improve the referenced core patch and write tests there and push it for 8.1. That's something we should do anyway I think. Try to move more tokens and features from this module into core.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
interdiff-5-8.txt1.27 KB
fix_or_remove_unused_form_alter-2641226-8.patch3.05 KB

Addressing comments in #4. Tests pending.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Needs work

needs work for some basic tests, paragraph #2 from comment #4 about the user form is also not addressed yet.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

Point 2 from #4 is done:

+++ b/token.module
@@ -107,29 +107,6 @@ function token_theme() {
-    case 'system_add_date_format_type_form':
-    case 'system_delete_date_format_type_form':
-      $form += array('#submit' => array());
-      array_unshift($form['#submit'], 'token_clear_cache');
-      break;

@@ -213,6 +180,20 @@ function token_field_display_alter(&$display, $context) {
+ * Implements hook_date_format_insert().
+ */
+function token_date_format_insert() {
+  token_clear_cache();
+}
+
+/**
+ * Implements hook_date_format_delete().
+ */
+function token_date_format_delete() {
+  token_clear_cache();
+}

This is what you had in mind, right?

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented

That part is fine, I meant paragraph 2, not point 2:

However, I did find a small bug in a form alter function, lets fix this here too. token_form_user_admin_settings_alter() has the original string that it replaces (which makes sense, so it works for translated strings). But that string changed in core, so that doesn't work anymore.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

Oh, I changed that as well. It seems I lost it in patches. I will add that too. Thanks for reminding. :)

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented

Just FYI, I found a related issue in core here. I filed #2642674: Tokens not replaced in file/widget field descriptions.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Related issues: +#2642674: Tokens not replaced in file/widget field descriptions
hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
interdiff-8-15.txt4.23 KB
fix_or_remove_unused_form_alter-2641226-15.patch7.18 KB

Adding a few tests and fixing the comment in #11.

Status: Needs review » Needs work

The last submitted patch, 15: fix_or_remove_unused_form_alter-2641226-15.patch, failed testing.

hussainweb’s picture

hussainweb
Primary language English
CreditAttribution: hussainweb at Axelerant commented
Status: Needs work » Needs review
FileSize
interdiff-15-17.txt506 bytes
fix_or_remove_unused_form_alter-2641226-17.patch7.21 KB

Fixing...

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Needs review » Fixed

Nice work, committed.

  • Berdir committed 14e7d49 on 8.x-1.x authored by hussainweb 
    Issue #2641226 by hussainweb: Fix or remove unused form alters

Status: Fixed » Closed (fixed)

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