Machine names are too hard to implement. Date types and menu names are not validated

Now my version:

different to suns patch:

// Verify that using a menu_name that is too long results in a validation message.
    $this->assertRaw(t('!name cannot be longer than %max characters but is currently %length characters long.', array(
      '!name' => t('Menu name'),
      '%max' => MENU_MAX_MENU_NAME_LENGTH_UI,
      '%length' => drupal_strlen($menu_name),
    )));

// Verify that no validation error is given for menu_name length.
    $this->assertNoRaw(t('!name cannot be longer than %max characters but is currently %length characters long.', array(
      '!name' => t('Menu name'),
      '%max' => MENU_MAX_MENU_NAME_LENGTH_UI,
      '%length' => drupal_strlen($menu_name),
    )));
    // Unlike most other modules, there is no confirmation message displayed.

%max -> <em class="placeholder">27</em>

This is a really great idea!

Subscribe. I'll have more to say later. Looks nice so far.

Interesting - making providing machine names simpler would be definitely useful.

I just had a short look at the patch:

* The naming of #machine_name_exists doesn't make clear it's a callback when reading the FAPI structure.

Question:

+ // Store a reference to the complete form in $form_state prior to building
+ // the form. This allows advanced #process and #after_build callbacks to
+ // perform changes elsewhere in the form.
+ $form_state['complete form'] = &$element;

Why is that necessary? The process / after_build callbacks are run on the cached copy again nevertheless.

Subscribe

very cool ... subscribing

+++ includes/form.inc	17 Sep 2010 00:07:51 -0000
@@ -2911,7 +2916,111 @@ function form_process_tableselect($eleme
+  // Apply default form element properties.
+  $element += array(
+    '#title' => t('Machine-readable name'),
+    '#description' => t('A unique machine-readable name. Can only contain lowercase letters, numbers, and underscores.'),
+    '#machine_name_source' => array('name'),
+    '#machine_name_label' => t('Machine name'),
+    '#machine_name_pattern' => '[^a-z0-9_]+',
+    '#machine_name_replace' => '_',
+  );

any reason these are not in hook_element_info()?

+++ includes/form.inc	17 Sep 2010 00:07:51 -0000
@@ -2911,7 +2916,111 @@ function form_process_tableselect($eleme
+  $js_settings = array(
+    'type' => 'setting',
+    'data' => array(
+      'machineName' => array(
+        '#' . $source['#id'] => array(
+          'text' => $element['#machine_name_label'],
+          'target' => '#' . $element['#id'],
+          'pattern' => $element['#machine_name_pattern'],
+          'replace' => $element['#machine_name_replace'],
+          'suffix' => '#' . $suffix_id,
+        ),
+      ),
+    ),
+  );

are we worried about multiple machine name elements on same page. seems like their js settings would collide. this is a minor issue and not a new problem so should be addressed elsewhere (if needed).

once tests pass, this is rtbc IMO.

I do think it is more consistent to consolidate to a single #machine_name key with various properties. This would match how #cache and #attached look. I also think it is consistent to move to system_element_info(). Thanks.

#28: drupal.machine.28.patch queued for re-testing.

Looks good to me and to the bot.

To summarize, we have a bug where Date Type (whatever that is) has no machine name validation at all. Rather than add yet another custom implementation, sun made a generic #machine_name FAPI element and converted menu, vocab, etc. to use it. Thats a terrific bug fix in my book. It fixes the core bug and prevents many more in Contrib.

@tha_sun - a different issue related to element defaults is now at #914792: Custom element properties entirely override default element info properties

++ Fantastic developer experience improvement and all-around code rationalization.

This could help with #606598: Human readable image-style names

I see nothing in this issue that justifies API changes of this nature 12 months after API freeze. Sounds like a great feature for D8 though.

@webchick - i posted a summary in #31 where i try to answer that question. i can't tell if you missed it or you disagree.

I read it. It's very difficult for me to justify a 45K patch that changes multiple parts of the system when the actual fix for the bug that was accidentally found as a result of adding this feature 12 months after API freeze is like a line or two of code.

If Dries thinks otherwise, that's fine, but it's a no from me.

+1 for this feature. And a back port to 6 even if core doesn't use it ;)

I'm also curious why this couldn't be implemented in contrib, with hook_element_info()? Then Drupal 6 really could use it.

The case as to why this patch is needed was made well in #41 and elsewhere in this issue. Can we get this in and move on to #903730: Missing drupal_alter() for text formats and filters before we're stuck with these problems for another release?

We have a period of the release, called "code thaw" to put in new features like this. That period lasted 18 months and ended last September. There was not then, and isn't now, an exception to code thaw to build exportable support into Drupal 7 core. We are currently actively trying to get Drupal 7.0 out the door, not keep stuffing more stuff in to an already over-stuffed release.

sun's argument for this patch seems to be "security," which is the only reason I'm leaving it for Dries to make a call on, and not immediately returning it to D8 where it actually belongs. The arguments for any other exportable-related patches are on incredibly shaky ground from my POV. If there are patches in the queue at this point that don't directly lead to Drupal 7.0 shipping faster and more bug-free, then they're all D8 as far as I'm concerned. Hopefully those concerned about core exportable support can rally quickly during the D8 code thaw to ensure it's fixed for good then.

So, no. We can't "get this in" and "move on" to other exportable issues. Unless you and Dries have some secret pact I don't know about that exempts exportable-related features from feature freeze.

@webchick - I apologize if my comment came off as trying to skirt the rules of code freeze. I understand that freeze was ages ago. However, various other issues dealing with inconsistencies and "WTF" DX issues have been committed. I hope that an exception is made here based on the value of this fix relative to the extent of the changes. Contrib is hurting because core does not handle machine names in a consistent manner.

Yes, it's very late for API changes, and this issue and the accompanying #903730: Missing drupal_alter() for text formats and filters have the potential to further drag on D7 readiness. However useful , these changes should be considered only if there is a compelling case that D7 would be effectively broken without them.

Here's the case as I understand it.

Text formats are a key element of Drupal's security system. They are also one of the most complex, difficult, and error-prone areas of site configuration, particularly if a WYSIWYG editor is in use. Current stats for Fckeditor, Ckeditor, WYIWYG, and Drupal as a whole suggest that more than half of all D6 sites (~178,000) have a WYSIWYG editor in use. WYSIWYG editors generate a range of tags, some of which require specific attributes, some of which are not allowed by the drupal core tag filter. Configuring a rich text editor to work with an input format is difficult and frustrating, even with specialized contrib tools like WYSIWYG Filter.

As a result, an unknown but probably large number of site admins throw up their hands and abandon tag filtering altogether, allowing full HTML for the simple reason that they can't figure out how to get WYSIWYG to work without it. I know that I personally have encountered multiple sites with anonymous comment input on which full html had been set as the default input format purely because site admins couldn't figure out how to configure a filter for rich text editing.

The only effective way to address this issue is going to be through exportables. By enabling text formats to be exportable, we can provide polished configurations that achieve a high level of functionality without sacrificing security.

The Features module is the leading solution for distributing exportables, but input formats are not supported. Actually, the code is there in Features, but by design it is inactive. The reason is that input formats lack a machine name. This lack means not only that text formats themselves can't effectively be exported. Equally important, it means that associated data - WYSIWYG configurations, filter configurations, default formats per role, etc. - are keyed by format ID in numerous places throughout core and contrib.

There have been numerous efforts to solve this issue in contrib. None could be called successful. A recent article demoed WYSIWYG as a Feature, but a closer examination of the patch used leads to a tangled patchwork of half solutions.

Only a fix in core can provide a clean D7 solution to this significant security concern.

As a side benefit, exportable filter formats will help address a serious usability barrier, by making it possible to provide richly featured WYSIWYG and other text format-based Features.

And as for why this patch is so late coming, the reason is plain: sun was plenty busy with other contributions, and - despite the plethora of commentators - no one else had the clarity of thought to solve this issue at its source.

Does all this add up to sufficient basis for a code freeze exception? It's hard for me to judge. I am convinced, though, that once again sun has provided the combination of insight and excellent coding needed to fully address a glaring need in core.

Personally, I can commit to reviewing any follow up patches that may be required.

[Edited to finish an incomplete sentence.]

This kick to 8.x is hard to watch. I hope Drupal adopts a philosophy of making code rationalization and API additions to major versions even after release, and we will see this - and other work, especially sun's, in 7.

sun++

Re my comments in #47, today I posted an attempt at the best workaround available currently for D6/D7 to address WYSIWYG feature: Debut WYSIWYG. It's a good illustration of what we need to fix, but I guess it also demonstrates that, with many hacks and limitations, we can do something in the meantime.

[edited to correct the link]

Yeah, I'd be willing to take a look at what a patch like that looks like. That sounds like a more under-the-hood change, as opposed to an API change that would affect module developers directly.

Looks good. Everything works, date type machine name doesn't let me use all caps, etc. Ready for webchick, at least!

OK, this one even more focused on the bug fix and standardizing these elements for Contrib. I much agree with Sun's case. Please take another look.

subscribing

The new patch looks better. Now there are effectively no FAPI changes, but a small improvement. Similarly the patch helps a lot when introducing machine names. As I've done some machine name form elements already for D7 I very much agree that *properly* doing this machine name form element is way too complicated now.

+    'pattern' => '[^a-z0-9_]+',

Hm, without reading the docs I'd assume this is the pattern for valid chars. But it is the pattern for finding non valid characters.

Looking up the docs, they are wrong:

+ *     - pattern: (optional) A regular expression (without delimiters) matching
+ *       valid characters in the machine name. Defaults to "[^a-z0-9_]+".

Anyway, what about using [a-z][a-z0-9_]* as default pattern for valid machine names? That way we'd ensure they start with a letter and disallow solely numeric machine names. First of numeric machine names don't make much sense. Secondly this allows for using a "magic" $identifier variable, which may contain either the machine name or the id + writing a loader function that just checks with is_numeric() whether to load by numeric id or machine name. Having this in place, any references to the entity can be easily changed to machine names or just kept by ids.

Summary is in #62

Agreed that [a-z][a-z0-9]* is out of scope.

+  $element['#machine_name'] += array(
+    'source' => array('name'),
+    'target' => '#' . $element['#id'],
+    'label' => t('Machine name'),
+    'pattern' => '[^a-z0-9_]+',
+    'replace' => '_',
+  );

As said, I'd assume without reading the docs 'pattern' describes the regex for valid machine names. Perhaps better rename it to 'replace_pattern' to make this more clear?

"pattern" is the name of the HTML5 attribute that does the same thing (validate based on a regex), so I think it's fine to use the same name (especially if we are able to later expand that same property for more HTML5 goodness later).

HTML 5 pattern does exactly what one would assume for "pattern", it states the pattern that the element has to fulfill. See here.

But the #machine_name pattern specifies the characters to remove/replace. Perhaps we can rewrite the JS to work like the HTML 5 pattern? That would be certainly the best DX and potentially allow use to make use of the mentioned HTML5 goodness later.

Hm. fago is correct; I misread the patch when I was just skimming the issue. Given that it is a post-process pattern rather than a validation pattern it may be wise to name it as such to avoid collision later (with the html5 project and similar efforts that would likely want to add such a property if they can).

This patch looks good.

+++ includes/form.inc
--- /dev/null
+++ misc/machine_name.js

The other files use dashes instead of underscores. For example misc/vertical-tabs.js.

Patch looks good to me too. 'replace_pattern' is much better and tells me what it is about really, thanks.

recap of #62:
1. appears to be done
2. This appears to be solved for menu. AFAICT the remaining work is deferred to other tickets waiting on this patch.
3. appears to be done

It appears that #75 is the only potential remaining issue, is that correct?

If so I think the rework from #78 avoids any ambiguity and should address the concerns presented by crell in #77. This appears to be a reasonable interim solution and if a better solution is desired we appropriately defer to contrib to flesh out the details.

This patch fixes a bug, but is also a last minute clean-up that will help with better distribution support. We discussed this in #933846: Better distro support.

Committed to CVS HEAD. Thanks!

Nice, how do we get that to http://api.drupal.org/api/drupal/developer--topics--forms_api_reference.... ?

This broke HEAD - machine-name.js doesn't seem to have gotten committed.

Oops. :P Committed that to HEAD.

A system module hunk flew by too. Hope that didn't break anything.

Resetting status back due to #87

Just chatted with rfay in IRC and I am working on a patch for the documentation issues. Will post for feedback.

Adjustments to forms_reference_api.html to address #87

Attached is a diff and the complete html file please review.

@jgraham, the patch is pretty hard to review. Could you just put the relevant updated HTML (the main content part) into this issue? Alternately, you could put the html file somewhere and give us a link to it.

Thanks!
-Randy

@rfay: Here's a copy-paste from the zip file in #92 -- hope it helps.

@jgraham: I checked the below "Properties" against the above table and found some discrepancies:

1. #required isn't checked in the table, but is listed under Properties
2. #type is listed as a property, but I believe you meant #weight (edit: or perhaps you meant both #type and #weight? Anyway, #weight is checked in the top table)
3. #title and #type are in <strong> tags

machine_name

Description: Provides a unique machine-readable name.

By default provides a machine name that is validated such that it contains only alphanumeric and '_'. All characters not meeting this criteria are replaced with '_'. The validation routines also fail if the element value is comprised solely of '_'.

Non-standard form element properties:

• #machine_name: (array)
  • exists: A function name to invoke for checking whether a submitted machine name value already exists. The submitted value is passed as argument. In most cases, an existing API or menu argument function can be re-used. The callback is only invoked, if the submitted value differs from the element's #default_value
  • source: (optional) The #array_parents of the form element containing the human-readable name (i.e., as contained in the $form structure) to use as source for the machine name. Defaults to array('name').
  • label: (optional) A text to display as label for the machine name value after the human-readable name form element. Defaults to "Machine name".
  • replace_pattern: (optional) A regular expression (without delimiters) matching disallowed characters in the machine name. Defaults to '[^a-z0-9_]+'.
  • replace: (optional) A character to replace disallowed characters in the machine name via JavaScript. Defaults to '_' (underscore). When using a different character, 'replace_pattern' needs to be set accordingly.

Properties: #access, #after_build, #array_parents, #attached, #description, #disabled,#element_validate, #parents,#post_render, #prefix,#pre_render, #process,#required, #states,#suffix, #theme, #theme_wrappers,#title, #tree,#type

Usage example (menu.admin.inc):

$form['menu_name'] = array(
  '#type' => 'machine_name',
  '#title' => t('Menu name'),
  '#default_value' => $menu['menu_name'],
  '#maxlength' => MENU_MAX_MENU_NAME_LENGTH_UI,
  '#description' => t('A unique name to construct the URL for the menu. It must only contain lowercase letters, numbers and hyphens.'),
  '#machine_name' => array(
    'exists' => 'menu_edit_menu_name_exists',
    'source' => array('title'),
    'label' => t('URL path'),
    'replace_pattern' => '[^a-z0-9-]+',
    'replace' => '-',
  ),
  // A menu's machine name cannot be changed.
  '#disabled' => !empty($menu['old_name']) || isset($system_menus[$menu['menu_name']]),
);

@claar: thanks for making that more readable to everyone.

Adjusted as per claar's comments in #95
1. The table was an omission.
2. I meant #type and #weight
3. fixed

Well. That explains why I had extra hunks in that commit I did just before hopping on a phone call. :D Thanks!

Committed #98 to HEAD. Back to needs review for docs?

Re-tagging.