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.
There's no real need for #attributes to be set for theme_container(), there's no harm in letting developers render a div without attributes.
Errors like this just end up encouraging #prefix/#suffix abuse.
There's no BC break here, just a minor cleanup.
Comment | File | Size | Author |
---|---|---|---|
#22 | 2058761-22.patch | 4.42 KB | kirby14 |
#5 | 2058761-1-5-interdiff.txt | 853 bytes | thedavidmeister |
#5 | 2058761-5.patch | 5.53 KB | thedavidmeister |
#1 | 2058761-1.patch | 5.53 KB | thedavidmeister |
Comments
Comment #1
thedavidmeister CreditAttribution: thedavidmeister commentedtests + fix.
Comment #2
jibranNice fix. I think we should backport it.
Comment #3
Fabianx CreditAttribution: Fabianx commentedtagging for backport
Comment #4
alexpottI've confirmed the new
Drupal\system\Tests\Common\RenderElementTypesTest
fails when the patch is not applied to theme.incShould be "Contains..."
HTML :)
Comment #5
thedavidmeister CreditAttribution: thedavidmeister commentedhope you don't mind me setting this back to RTBC with the typo and doc fix.
Comment #6
alexpottCommitted 5d3b946 and pushed to 8.x. Thanks!
Comment #7
star-szrProbably not worth reopening for 8.x but should be "Contains \Drupal…" for next time.
Moving to 7.x for the backport.
Comment #8
thedavidmeister CreditAttribution: thedavidmeister commentedMaking a backport is a novice task i think.
Comment #9
kirby14 CreditAttribution: kirby14 commentedI got the core code, since it is only 2 lines, but I'm a little confused on how to back port the test case.
I see you removed a file and created a new one for D8, should I be looking to do the same in D7? It looks like it is set up a little different.
Comment #10
thedavidmeister CreditAttribution: thedavidmeister commentedhmmm, how different is the setup? ideally you'd want the changes to be 1:1, but if that's not possible we should have a quick discussion about why we can't do it then use common sense to finish this up.
Comment #11
kirby14 CreditAttribution: kirby14 commentedI just wasn't looking in the right spot. Looks like it is in theme.test instead of common.test in D7. I'll spend some time working on it.
Comment #12
thedavidmeister CreditAttribution: thedavidmeister commentedgreat, thanks!
Comment #13
kirby14 CreditAttribution: kirby14 commentedOk, let's give this a shot.
Comment #14
kirby14 CreditAttribution: kirby14 commentedI guess I should slow down before submitting the patch. Last one was only the test, didn't include the actual fix. This one should do.
Comment #15
kirby14 CreditAttribution: kirby14 commentedComment #18
kirby14 CreditAttribution: kirby14 commentedReally sorry about spamming the log. I'll get the hang of this.
Comment #19
kirby14 CreditAttribution: kirby14 commentedComment #21
kirby14 CreditAttribution: kirby14 commentedOk, this one runs locally. I assume using a #type => html_tag is equivalent to calling theme_html_tag directly, since that is what is in the original patch.
D7 doesn't appear to support #markup so I used a child array instead for the container tests.
Comment #22
kirby14 CreditAttribution: kirby14 commentedLast patch somehow had a spacing issue that didn't show up in my IDE. Is there a way to run dreditor over a patch before posting?
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentedYou could try https://drupal.org/project/coder there are other ways too, you should be able to configure your IDE to automatically clean up your code, or at least warn you about problems.
Comment #24
mgifford@thedavidmeister - any concerns about the latest patch? Does it still need to be cleaned up?
It still applies nicely and would be good to avoid these notice's if we can.
Comment #25
dcam CreditAttribution: dcam commented#22 looks ok to me.
Note that the D7 test has one fewer assertions than in D8. As mentioned in #21, D7 doesn't support #markup in container render arrays.
Also, note that the D8 patch changed the focus of the test class, from saying that it tests HTML tags to testing element render arrays. The D7 patch does the same thing. A lot of the code gets moved around and formatted differently to match D8. I checked it and I think it's all good.
Comment #28
dcam CreditAttribution: dcam commentedYet another random test failure.
Comment #31
dcam CreditAttribution: dcam commentedComment #34
dcam CreditAttribution: dcam commentedComment #37
dcam CreditAttribution: dcam commentedComment #40
dcam CreditAttribution: dcam commentedComment #43
dcam CreditAttribution: dcam commentedComment #44
Fabianx CreditAttribution: Fabianx commentedI agree with RTBC for that.
Comment #45
David_Rothstein CreditAttribution: David_Rothstein commentedCommitted to 7.x - thanks!
Nice cleanup work on those tests, too.