Problem/Motivation

To quote \Drupal\Core\Render\Element\MachineName:

exists: A callable to invoke for checking whether a submitted machine name value already exists. The submitted value is passed as an argument. In most cases, an existing API or menu argument loader function can be re-used. The callback is only invoked if the submitted value differs from the element's #default_value.

This doesn't document the remaining two parameters used in the callback:

call_user_func($function, $element['#value'], $element, $form_state)

This recently came up where some code was assuming parameter 2 was the full form, and not just the form element, and the only way to figure it out was to dive into validateMachineName() method.

Proposed resolution

Describe the additional parameters in the documentation.

Remaining tasks

Commit the patch

User interface changes

None

API changes

None

Data model changes

None

CommentFileSizeAuthor
#27 document_parameters-2173831-27.patch886 bytesvegantriathlete
#27 interdiff.21-27.txt730 bytesvegantriathlete
#21 document_parameters-2713831-20.patch891 bytesvegantriathlete
#21 interdiff.19-6.txt693 bytesvegantriathlete
#19 document_parameters-2713831-19.patch891 bytesjoshi.rohit100
#6 document_parameters-2713831-6.patch890 byteslisa.ugray
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

deviantintegral created an issue. See original summary.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Issue tags: +Novice

Thanks! I think this would be a good Novice project.

kellyimagined’s picture

kellyimagined CreditAttribution: kellyimagined commented

We will begin looking at this issue

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
CreditAttribution: vegantriathlete as a volunteer commented

We will be adding a proper issue summary and we will roll a patch for the docblock.

marlatt’s picture

marlatt CreditAttribution: marlatt commented
Version: 8.1.x-dev » 8.2.x-dev
lisa.ugray’s picture

lisa.ugray CreditAttribution: lisa.ugray at University of Ottawa commented
FileSize
document_parameters-2713831-6.patch890 bytes

Added missing callback parameters to the documentation.

selfuntitled’s picture

selfuntitled CreditAttribution: selfuntitled commented
Status: Active » Needs review
kellyimagined’s picture

kellyimagined CreditAttribution: kellyimagined commented
Issue summary: View changes
lisa.ugray’s picture

lisa.ugray CreditAttribution: lisa.ugray at University of Ottawa commented
Version: 8.2.x-dev » 8.1.x-dev
lisa.ugray’s picture

lisa.ugray CreditAttribution: lisa.ugray at University of Ottawa commented
Issue tags: +neworleans2016
munizjor’s picture

munizjor CreditAttribution: munizjor commented

Issue tag has been reverted back to 8.1, but the patch will be applied to 8.2. Will review after lunch.

soulsesa’s picture

soulsesa CreditAttribution: soulsesa commented
Status: Needs review » Reviewed & tested by the community

Previously, only the submitted value was documented. Now the submitted value, form element array and form state have been documented.

akalata’s picture

akalata CreditAttribution: akalata commented
Status: Reviewed & tested by the community » Needs work

I'm not sure that this issue is RTBC yet. Are these arguments being passed as an array? If so, do they match up with expected variables or key values?

lisa.ugray’s picture

lisa.ugray CreditAttribution: lisa.ugray at University of Ottawa commented

The callable is being passed in. These are the arguments that will be passed to the callable.

lisa.ugray’s picture

lisa.ugray CreditAttribution: lisa.ugray at University of Ottawa commented
Status: Needs work » Needs review
vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
CreditAttribution: vegantriathlete as a volunteer commented
Status: Needs review » Reviewed & tested by the community

The point of the patch was to add documentation about the parameters that are being passed via call_user_func(). As stated in the issue summary:

      $function = $element['#machine_name']['exists'];
      if (call_user_func($function, $element['#value'], $element, $form_state)) {

is the section of the code we are documenting. The original docblock mentioned only the submitted value parameter. It did not mention the other two parameters. The attached patch has included the mention of the other two parameters.

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
CreditAttribution: vegantriathlete as a volunteer commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -22,7 +22,10 @@
+ *     - The form state array.

This is not an array - it is an object that implements FormStateInterface

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Needs work » Needs review
FileSize
document_parameters-2713831-19.patch891 bytes

Status: Needs review » Needs work

The last submitted patch, 19: document_parameters-2713831-19.patch, failed testing.

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
CreditAttribution: vegantriathlete as a volunteer commented
FileSize
interdiff.19-6.txt693 bytes
document_parameters-2713831-20.patch891 bytes

@alexpott Thanks for the clarification!

@joshi.rohit100

Thanks for submitting a new patch! Here are a couple of points to consider:

  • It would have been better for you to include an interdiff
  • The patch should be submitted to test against 8.2.x
vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
CreditAttribution: vegantriathlete as a volunteer commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: document_parameters-2713831-20.patch, failed testing.

The last submitted patch, 21: document_parameters-2713831-20.patch, failed testing.

lisa.ugray’s picture

lisa.ugray CreditAttribution: lisa.ugray at University of Ottawa commented
Status: Needs work » Reviewed & tested by the community

Test failure was due to the branch, not the patch, as shown by retesting. Thanks @alexpott for that catch - I'm too used to D7. The latest patch looks good to me.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Reviewed & tested by the community » Needs review

Looking pretty good...

+++ b/core/lib/Drupal/Core/Render/Element/MachineName.php
@@ -22,7 +22,10 @@
+ *     - The form element array.

... but I think this is slightly confusing. See issue summary -- this issue was filed because of confusion between the form and the element. How about leaving the word "form" out here?

vegantriathlete’s picture

vegantriathlete
he/him/his
Primary language English
Location Lakewood, CO
CreditAttribution: vegantriathlete as a volunteer commented
FileSize
interdiff.21-27.txt730 bytes
document_parameters-2173831-27.patch886 bytes

Status: Needs review » Needs work

The last submitted patch, 27: document_parameters-2173831-27.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs work » Reviewed & tested by the community

Test failure seems to be unrelated. The patch looks good to me. Thanks!

deviantintegral’s picture

deviantintegral CreditAttribution: deviantintegral at Lullabot commented

Looks good to me as well. Thanks!

  • webchick committed 1eb935f on 8.2.x 
    Issue #2713831 by vegantriathlete, lisa.ugray, joshi.rohit100, jhodgdon...

  • webchick committed a4f9560 on 8.1.x 
    Issue #2713831 by vegantriathlete, lisa.ugray, joshi.rohit100, jhodgdon...
webchick’s picture

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

Committed and pushed to 8.2.x and cherry-picked to 8.1.x. Thanks!

Great to see the team work going on here. :)

Status: Fixed » Closed (fixed)

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