Submit buttons for GET forms in search/views are not W3C valid due to empty 'name' attribute

Imo, the url should stay as it's now.
Isn't it just an option to let the empty name stay, and make sure the name attribute isn't rendered when it's empty (for all elements, so not only this search submit)? This will prevent that other modules also introduce w3c invalid html.

ill work on this

added the name attribute based on the standard name in drupal 8 to avoid empty #name.

'#name' => 'search_block_form_submit',

@dom, thank you.

more checking :)

I marked this issue as related to #1366020: Overhaul SearchQuery; make search redirects use GET query params for keywords. Those following this issue should read that.

Um, wrong component. ;)

So. The reason that we put #name='' is shown in the comment just before the patched line from #8 and reflected in the test failure. That is, we do NOT want the submit button showing up in the GET parameters.

This patch breaks that, and in fact having any name in there breaks that.

I do not think there is a way to have a non-empty name and avoid having an ugly search URL. So I think this is a Won't Fix. But if someone can figure out how to have the Search block pass w3c validation and still pass the test that checks that the search URL is clean, sure...

Maybe #6 will work? Try it...

Unassigning @mikeocana as it has been a month without any activity. This will be my first patch for Drupal core.

Hey @jhodgdon
I took a look at the code that generates the markup for the input fields. I was able to add some logic that will check for empty name attributes and if empty would remove them. I have run my code through the W3C validator to ensure the resulting HTML for the submit button is now valid.

This is my first patch to Drupal core.

Hey @Dom,

I was helping @laranajim with this patch, and totally agree with your point #1, I will ensure a re-roll takes place tomorrow for this. As for point #2, $variables['attributes']['name'] is an object of type Attribute, the type cast seems to make it work reliably, but open to suggestions for ways to improve this (this is my first time working with this class).

Thanks for your time with the review! Very comprehensive.

This patch looks like a good solution to me. It has rather far-reaching consequences though (way beyond the Search module), so I'm moving this into the Form component.

Thanks for the research! Updating title and issue summary, and I think this is RTBC.

This is an issue in D7 too.

Hey @BarisW, cool, let's backport to D7 after being committed to D8.

So we should probably tag this issue as "needs backport to D7" and mark the other one as a duplicate then???

Maybe not - is the other issue fixed in Views?

The idea would be to fix the underlying problem, which also exists in D7, and needs to be fixed in Core because it is a Core form API problem.

Patch looks fine, but this could use automated testing.

What do you suggest for additional automated testing? There is already quite a bit of testing of the Search form, which earlier patches failed (there are tests for instance that verify that the form ID stuff doesn't get into the URL when you submit the form with GET).

I don't believe we can test for W3C validity easily in an automated test...

Maybe we could just make a short test for this particular issue that would verify that there is not an empty 'name' attribute in the form?

Looks good to me, thanks! Verified the tests failed/passed as expected.

+++ b/core/modules/search/src/Tests/SearchBlockTest.php
@@ -45,6 +45,11 @@ public function testSearchFormBlock() {
+    $this->assertTrue(empty($elements), 'The search input field does not have empty name attribut.');

Minor typo (attributE).

OK! Thanks BarisW for being more detail-spotting than the rest of us.

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

This patch is a direct backport for Drupal 7.

The next attempt.

Test only patch. Single failure is expected.
Oops. Something went wrong.

Hm. Does Drupal 7 even have the same problem? Maybe not since the test didn't fail...

The problem exists for Views exposed forms, see #2514028: Empty 'name' value for the exposed filter submit button.

In D7, default value for 'name' attribute of a button is 'op', see system_element_info().
So, unless you specify it explicitly as empty string, the resulting HTML code is valid.
Views exposed forms do exactly this in views_exposed_form() function in order to "prevent from showing up in $_GET".
This means the test must contain creating a special form with a button having #name set to empty string.

Ah, so in D7 this problem doesn't affect the Search form I guess. Anyway, sounds like you know how to modify the test. Setting to Needs Work because the test needs work.

Can confirm patch in #44 works nicely in 7. Thanks.

The test needs work. See earlier comments.

Just a little more info on this as I've just come across the issue.

I have a views exposed filter with the name="" as expected. As discussed it doesn't pass W3C validation and I've also noticed that there is different behaviour between browsers.

On a windows machine Chrome doesn't include the button value in the search string but in IE it's included but without a name. For example with me it was appending &=Search in IE for the search button.

This means that as far as caching is concerned the pages are not the same and therefore need caching again.

For some reason it also means that return has to be pressed twice in IE when entering a search term.

The patch in #44 appears to fix all of the issues for me so it's a thumbs up from me.

I'd like to take this up! :)

I'd commented in #55 that

for some reason it also means that return has to be pressed twice in IE when entering a search term.

After more testing I don't think it's related to this issue and more to do with a browser quirk if you enter a term that has already been entered previously but I can't replicate it every time.

Either way it's not to do with the original issue.

Are there any tasks left on this issue?

Whats the latest on this ticket. Looks like there has been commits but never marked as fixed.

We are seeing this on our distribution, just interested to know if this could be the issue or if this is considered fixed.

D7 should be fixed also.

Doesn't look fixed based on my WC3 site validation check, and yet supposedly catch has committed a fix. (#59)

Tagging for upcoming contribution days.

This issue still remains with drupal 8.6.8 and needs to fixed.
Here is the error I receive in w3c validator:

Error: Bad value for attribute name on element button: Must not be empty.

From line 207, column 300; to line 207, column 412

roup-btn"><button type="submit" value="Search" class="button js-form-submit form-submit btn-primary btn icon-only" name=""><span 

This issue remain with 8.6.8 and needs to be fixed.

w3c validation returns error :
Bad value for attribute name on element button: Must not be empty

I tried a work-around for this issue. I am not sure if it is the right approach or not.

As the 'name' attribute is NULL for the html tag , it is causing the W3C validator to issue the following error:
Error: Bad value for attribute name on element button: Must not be empty.

In order to deal with it, I created a 'block--theme-search.html.twig' file in the templates folder of the sub-theme.
This overrides the core code.

I particularly removed the attribute 'name' and error is gone.

Here is code for the 'block--theme.search.html.twig' file:

<div class="search-block-form block block-search block-search-form-block" data-drupal-selector="search-block-form" id="block-simpleblogtheme-search" role="search">
  <h2 class="visually-hidden">Search</h2>
  <form action="/search/node" method="get" id="search-block-form" accept-charset="UTF-8">
    <div class="form-item js-form-item form-type-search js-form-type-search form-item-keys js-form-item-keys form-no-label form-group">
      <label for="edit-keys" class="control-label sr-only">Search</label>
      <div class="input-group">
        <input title="Enter the terms you wish to search for." data-drupal-selector="edit-keys" class="form-search form-control input-lg" placeholder="Search" type="search" id="edit-keys" name="keys" value="" size="15" maxlength="128" data-toggle="tooltip" />
         <span class="input-group-btn">
           <button type="submit" value="Search" class="button js-form-submit form-submit btn-primary btn icon-only">
             <span class="sr-only">Search</span>
             <span class="icon glyphicon glyphicon-search" aria-hidden="true"></span>
           </button>
         </span>
      </div>
    </div>
    <div class="form-actions form-group js-form-wrapper form-wrapper" data-drupal-selector="edit-actions" id="edit-actions"></div>
  </form>
</div>

As I mentioned earlier, I am not sure if its the right approach or not.
Needs to be tested and reviewed by the community.

Fixing tags

This issue does not apply for 8.6.x or 8.7.x. Definitely D8 does not present this issue.

I can confirm this issue does not exist in 8.5.x or higher. Setting back to 7.x-dev for back porting of patch. Patch #44 corrects the issue. Attached is a re-rolled copy against HEAD.

The test is no longer valid, however. Below is what the Search form markup looks like in 7.65:

<div class="form-actions form-wrapper" id="edit-actions"><input type="submit" id="edit-submit" name="op" value="Search" class="form-submit"></div><input type="hidden" name="form_build_id" value="form-fqsEapcJJVZpN9nno_Rkobvl6jJGPaxvHkJIS7TzY1Q">

Because of the change in the Search module, I tested with an exposed filter in views.

Leaving at Needs Work due to test coverage.

D7 branch tests are failing. Test failures are unrelated to patch #74.

Updated patch with updated tests.

I removed the Needs issue summary update after a few tweaks. I also removed the Needs Accessibility Review tag because the approach is already in 8.1.x. Ideally the back port should now be a follow-up issue these days, which may be why there was some confusion.

as per @alexpott #3061891: Validate Attributes (W3C validation)

This should be fixed in bootstrap, not core.

here is a bootstrap type of a fix that others have been using.

for more details, see @alexpott bootstrap patch and discussion in #3061891: Validate Attributes (W3C validation)

The issue can be reproduced without Bootstrap. There are two parts to patch #76 - the bug fix and updated tests. We can verify this is a core issue by running tests on a test-only version of the patch. This patch should fail. I'm uploading a test-only version of #76 and setting the project back to D7 core to allow tests to run.

Tests failed, as expected:

Form API.FormsElementsLabelsTestCase.testFormLabels

fail: [Other] Line 847 of modules/simpletest/tests/form.test:
No name attribute found.

This result confirms this issue is a Drupal core issue. Without this patch theme_button() will render a name attribute with no value. Setting back to Needs Review. Patch #76 remains the latest patch for review/commit.

Patch in #76 LGTM +1

As mentioned in earlier comments, this doesn't seem to affect D7 core directly because it doesn't use an empty name attribute to avoid unwanted GET params, and core's search forms use POST.

So perhaps the "empty name hack" only applies to views in D7? (and possibly unknown other contrib modules, custom code etc..?)

https://git.drupalcode.org/project/views/-/blob/7.x-3.24/views.module#L2163

  // Form submit, #name is an empty string to prevent showing up in $_GET.
  $form['submit'] = array(
    '#name' => '',
    '#type' => 'submit',
    '#value' => t('Apply'),
    '#id' => drupal_html_id('edit-submit-' . $view->name),
  );

I'm not entirely sure we should be "fixing" this in core?

I've manually tested with a views exposed filter and confirmed that the empty name attribute is removed with the patch, and that the GET params when the search form is used are unaffected.

However, why was this hack put there in the first place then? Looks like without views setting an empty name, core provides a default name="op" which does then pollute the GET params.

(Sorry for probably going over old ground but restating this makes it easier to review a 80+ comment issue that dealt with D8 and then became a D7 backport).

So this change is probably okay for D7 core to make as it's effectively removing an artefact of a hack that contrib code does to avoid core's default behaviour.

RTBC + 1, while not technically a core bug, core has always been friendly to contrib shenanigans :D.

There is no harm that I can see in removing an empty attribute and if themes are broken worst case can override theme_button() again.

Thank you!