Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently views forms are doubling up on the form tag.
Steps to reproduce:
- Visit /admin/content
- View Source
Proposed resolution
Determine where this got in, and remove duplicate.
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#12 | Test_result___Site-Install.png | 121.6 KB | joelpittet |
#10 | nested_form_tag_in-2558061-10.patch | 2.5 KB | joelpittet |
#10 | nested_form_tag_in-2558061-10-tests-only.patch | 1.2 KB | joelpittet |
#8 | Source_of__http___d8_dev_admin_content.png | 42.09 KB | joelpittet |
#7 | nested_form_tag_in-2558061-7.patch | 657 bytes | lauriii |
Comments
Comment #2
joelpittetWell this seems to work because it is already a form, and
#theme => form
is a#theme_wrapper
for#type => form
by default. Not totally sure about the removal of the #children thing in the preprocess but feels right. Needs manual testing for sure.
Comment #3
Aki Tendo CreditAttribution: Aki Tendo commentedTested. Redundant tag did disappear after patch was applied.
Comment #4
dawehnerits a bug, so we should have tests, sorry.
Comment #5
joelpittetNo need to apologize, you are right it does:) It's just a chore sometimes when bugs are easy to fix. And if you see my tests you'd regret asking for them, muahaha:D
I've got the assertion ready. Just need to build a view like admin/content that has the nested form.
There doesn't seem to be any Views Form Tests in core. May consider creating a new one
ViewsFormTest
Comment #6
joelpittetComment #7
lauriiiI cant manually reproduce this anymore. Neither do my tests :)
Comment #8
joelpittetMaybe the type of form is wrong, I can still preproduce this by view source on the admin/content page.
Comment #9
joelpittetAlso that test is incorrect, if you look at that output it doesn't really create one. But @dawehner let me know "Bulk Update" test may work so I'm going to try that.
Comment #10
joelpittetApparently my xpath() is crap or it just sucks at finding nested form tags... wrote a regex instead.
Comment #12
joelpittetFor the doubters of the xpath not working.
the xpath could be wrong... but I wanted to find also
/form/div/form
Comment #13
joelpittetComment #14
xjmCan we confirm when/how/why the children rendering was added in the first place? To make sure we're not fixing one usecase to break another or something.
Comment #15
cweagansIs there a reason to use a regex over a real HTML parser here? There's an HTML5-aware one in the masterminds/html5 package that's already in core. This would be a great place to use it.
Comment #16
joelpittet@cweagans thanks for the suggestion, didn't think to use that package, will give it a try. see #12 and #10 for why regex was used, because xpath didn't work as expected and I wanted to get something working and use something I know. But I'll give that package a try.
@xjm I know we want to remove it as it's one of the remaining "why is this being used" legacy function calls, and removing it helped resolve the problem.
a626abb - Add the 7.x-3.x Views branch. (2012-10-07 17:51:56 -0400)<merlinofchaos>
And we've removed all the other instance of that in core, so I think it's safe to remove.
Comment #17
joelpittet@cweagans Good news: Mastermind/html5 does a better job creating the DOMDocument for HTML5 and ignores errors(where SimpleTest just suppresses errors).
This means I can traverse the DOMDocument it creates much better than the one that SimpleTest creates. And I could test this problem with this:
Bad news: XPath doesn't work with HTML5, so the
DOMXPath()
basically finds nothing. Seems to be documented in this issue: https://github.com/Masterminds/html5-php/issues/12To see the work I did around this try it out for your self, maybe I made a mistake(very likely):
https://gist.github.com/joelpittet/62e7e5bd83e793c599c2
Output:
php < index.php
So with that, I'm going to stick with my simple regex because it's still more to the point and straight forward to read than loading the external library just to manually traverse, IMO.
Comment #18
cweagansThanks for looking into that. Given that going another route is so much extra code, I think the regex makes sense.
Comment #19
Aki Tendo CreditAttribution: Aki Tendo commentedYeah, for now I think this will have to do. Long term we need to find a different solution from XPath because HTML is not XHTML, there are many subtle differences. Browsers ignore them smoothly, but validators don't. One large examples - in HTML the /> ending on single tags is invalid, in XML and XHTML it's required. A prime example:
<br> vs. <br />
There's now an HTML 5 extension for Tidy, but the vast majority of libraries out there don't have tidy installed - those that do usually have the old html 4 version. Installing tidy is not trivial (unless you're using a virtual machine) so despite its power it's probably not usable with Drupal.
Comment #21
joelpittetLooks like a random failure. I applied to patch and ran "Drupal\user\Tests\UserPasswordResetTest" locally and it passed.
Comment #22
joelpittetComment #26
joelpittetComment #27
alexpottCommitted a7a9e36 and pushed to 8.0.x. Thanks!