Problem/Motivation

The following functions in field.module have 'private' namespaces indicated by the preceding underscore. These would be useful as public functions for contib modules to use.

Proposed resolution

For D8, rename the functions to remove the underscores from the names. Current patch is in #1174444-23: Make the _element_validate_* functions in field.module available for all contrib modules to use.

For D7, additionally provide backwards-compatible wrappers for these functions with the original names, and document that these variants are deprecated. Current patch is in #1174444-22: Make the _element_validate_* functions in field.module available for all contrib modules to use.

Remaining tasks

A broader discussion of the technique for "forwards compatibility" used here is in #1221904: RFC: forwards compatibility in D7 / 'backports' defgroup, so take this into consideration when evaluating the patch for D7.

User interface changes

None.

API changes

Minor, backwards-compatible addition only. No existing code will be affected.

New supported function names for validation functions:

  • element_validate_integer()
  • element_validate_integer_positive()
  • element_validate_number()

Since the backwards-compatible wrappers are still available (and the functions were previously "private" anyways), existing modules will not be impacted directly. However, modules should use the new function names moving forward.

CommentFileSizeAuthor
#23 1174444-23-D8-element-validate-public.patch7.09 KBxjm
#22 1174444-elment-validate-public-22.patch7.72 KBxjm
#19 1174444-element-validate-public-19-D7.patch7.72 KBboobaa
#10 1174444-element-validate-public-D7.patch5.87 KBboobaa
#1 1174444-element-validate-public.patch7.74 KBdave reid

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Assigned: Unassigned » dave reid
Status: Active » Needs review
StatusFileSize
new 1174444-element-validate-public.patch7.74 KB

Eventually we'll want to deprecate these and use the proper #numberfield with #min and #max properties/validation once it lands via #675348: META: Support HTML5 form input elements.

boobaa’s picture

boobaa
he/him
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs backport to D7

Subscribing; this would be warmly welcomed, eg. for #589440: Reordering fails with more than 31 book pages in a book, too. Additionally, I think this should even be backported to D7, at least.

boobaa’s picture

boobaa
he/him
commented
Issue tags: -Needs backport to D7

Correcting myself: the patch does not need to be backported to D7 as it applies cleanly, without offsets. Anyways, the RTBC still stands.

dries’s picture

dries commented

While this is an API change, I think it may be an acceptable one. It certainly makes sense to commit this patch to 8.x. Let's see what others have to say though.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: +API change, +Needs backport to D7

Actually this is not an API change, but an API addition, as the private functions are not part of the public API. Tagging as such, and re-tagging as backportable, so as not to forget.
RTBC +1

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue tags: -API change +API addition

I should listen to myself better!

boobaa’s picture

boobaa
he/him
commented

Don't know the exact meaning of "needs backport to D7" tag properly, but the patch in #1 applies cleanly to D7 as well, as mentioned in #3, without other side effects: the patch addresses all the occurences of the functions mentioned in OP.

tstoeckler’s picture

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

Tagging it as "needs backport to D7" is a quick indicator for reviewers and committers that this issue should be committed to D7 as well. It's not necessarily related to actually porting the patch.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Category: task » feature

I'd be ok with a D7 patch that didn't remove the old functions.

But this seems clearly to be a feature request, which means it gets looked at after our critical count gets down to <= 15.

boobaa’s picture

boobaa
he/him
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 1174444-element-validate-public-D7.patch5.87 KB

Here you are.

tstoeckler’s picture

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

I don't see why we shouldn't remove the old functions. People that call these functions are not using the public API. They are on their own and they know that. If we treat these functions as public API we should roll a patch for D8 to remove all underscores from all functions in core.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

They might know that, but their hapless users, who upgrade to Drupal 7.3 and are greeted with a fatal error they have no method of recovering from certainly won't know that.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Yeah I'm not in favor of removing them from D7. We could rename them and make the old private versions as a wrapper for the renamed public versions. Also doc the private ones as deprecated.

tstoeckler’s picture

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

It's against our naming convention, against every piece of documentation ever written, and against common sense, but knock yourselves out...

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Not sure what is wrong about #13:

/**
 * DEPRECATED: Helper form element validator: number.
 *
 * Use element_validate_number() instead.
 *
 * @deprecated
 * @see element_validate_number()
 */
function _element_validate_number(&$element, &$form_state) {
  return element_validate_number($element, $form_state);
}
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Component: base system » token system

Yep. #13/#15 is what I was thinking of. Not sure why this inspires such rage. ;)

tstoeckler’s picture

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

No rage involved :). Just don't expect me to roll this patch.

boobaa’s picture

boobaa
he/him
commented
Component: token system » base system

OK, I'm gonna roll this soonish; in the meantime I don't know why webchick changed component to token, so reverting. ;)

boobaa’s picture

boobaa
he/him
commented
StatusFileSize
new 1174444-element-validate-public-19-D7.patch7.72 KB

Here it is.

catch’s picture

catch
he/him
Primary language English
commented
Category: feature » task
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

This is blocking #589440: Reordering fails with more than 31 book pages in a book which is a major bug, and according to that issue Views is already using the private functions. Since there's a perfectly good patch there, it seems a shame to stall that one.

Patch looks completely fine to me, both the D7 patch in #19 and the D8 patch in #1.

Bumping to 'major' and task since it's holding up the book issue.

xjm’s picture

xjm
she/her
Primary language English
commented

Patch applies with offset to D8 and D7. I'll reroll it.

Edit: I also added an issue summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Added issue summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Clearer emphasis that it is a minor addition, not a change.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Added link to meta issue.

xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new 1174444-elment-validate-public-22.patch7.72 KB
xjm’s picture

xjm
she/her
Primary language English
commented
StatusFileSize
new 1174444-23-D8-element-validate-public.patch7.09 KB

#22 is for 7.x. This is a reroll of the patch for 8.x (which does not need the wrappers).

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes

Typo.

dries’s picture

dries commented
Version: 8.x-dev » 7.x-dev

Thanks all. I committed this patch to 8.x. Moving to 7.x so we can consider the back-ported patch with the extra wrappers.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

I think the patch makes sense for D7. But the wrappers are definitely necessary since Views (at least) is using the old names.

Committed and pushed #22 to 7.x. Thanks!

yched’s picture

yched commented

Completely missed that thread. W00t !

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.