Hello,

In https://cgit.drupalcode.org/bootstrap/tree/src/Plugin/ProcessManager.php..., the findButton() method does not find the nearest button as in the method PHPDoc, but the last one, as there is no break in the foreach when finding a button.

I found the problem in a form with multiple submit buttons.

I will provide a patch in a few hours and also testing on the last dev version of the theme.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper created an issue. See original summary.

Grimreaper’s picture

Version: 8.x-3.7 » 8.x-3.x-dev
Assigned: Grimreaper » Unassigned
Status: Active » Needs review
FileSize
869 bytes
3.05 KB
3.86 KB
3.04 KB

Tested on the last dev version.

There is still the bug.

I have uploaded a fix.

I have done tests modifying directly the native search block form to add submit button. www/core/modules/search/src/Form/SearchBlockForm.php:

    $form['keys'] = [
      '#type' => 'search',
      '#title' => $this->t('Search'),
      '#title_display' => 'invisible',
      '#size' => 15,
      '#default_value' => '',
      '#attributes' => ['title' => $this->t('Enter the terms you wish to search for.')],
    ];
    $form['keys']['test1'] = [
      '#type' => 'submit',
      '#value' => $this->t('test 1'),
    ];

    $form['actions'] = ['#type' => 'actions'];
    $form['actions']['test 2'] = [
      '#type' => 'submit',
      '#value' => $this->t('test 2'),
    ];
    $form['actions']['submit'] = [
      '#type' => 'submit',
      '#value' => $this->t('Search'),
      // Prevent op from showing up in the query string.
      '#name' => '',
    ];

bootstrap_button_1.png: the result without the patch.
bootstrap_button_2.png: the result with the patch.
bootstrap_button_3.png: the result with the patch and with the "test 1" button commented out.

Thanks for the review.

markhalliwell’s picture

Status: Needs review » Needs work

Interesting. Cannot believe I missed that heh

The findButton method really doesn't belong in ProcessManager anyway and should instead live in the Element class instead.

I think this patch should move the method (getting rid of the unnecessary $element parameter) and then deprecate this existing method?

Then it should be a simple as $button = &$parent->findButton() in processInputGroups

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
2.6 KB

Thanks @markcarver for the review.

Here is a new patch with your suggestion.

About the deprecation, I don't think it will be very frequent to have custom theme using the findButton method of the processManager. And maybe putting a warning in the next release note and maybe a change record will be good.

markhalliwell’s picture

Status: Needs review » Needs work

Don’t remove the method in ProcessManager, just deprecate it and return $element->findButton();.

Also, above that, call Bootstrap::deprecated().

Grimreaper’s picture

Status: Needs work » Needs review
FileSize
2.61 KB

OK... Now I see how you handle deprecation.

Here is a new patch.

Thanks for the review.

  • markcarver committed 7d58a10 on 8.x-3.x authored by Grimreaper
    Issue #2944760 by Grimreaper: Form preprocess not finding the right...
markhalliwell’s picture

Status: Needs review » Fixed
Grimreaper’s picture

Thanks for the commit :)

Status: Fixed » Closed (fixed)

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