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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

thedavidmeister’s picture

Status: Active » Needs review
Issue tags: -Novice
FileSize
5.53 KB

tests + fix.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Nice fix. I think we should backport it.

Fabianx’s picture

Issue tags: +Needs backport to D7

tagging for backport

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I've confirmed the new Drupal\system\Tests\Common\RenderElementTypesTest fails when the patch is not applied to theme.inc

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.phpundefined
@@ -0,0 +1,116 @@
+ * Definition of Drupal\system\Tests\Common\RenderElementTypesTest.

Should be "Contains..."

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/RenderElementTypesTest.phpundefined
@@ -0,0 +1,116 @@
+        'name' => "#type 'container' with a class HMTL attribute",

HTML :)

thedavidmeister’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
5.53 KB
853 bytes

hope you don't mind me setting this back to RTBC with the typo and doc fix.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 5d3b946 and pushed to 8.x. Thanks!

star-szr’s picture

Version: 8.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Probably not worth reopening for 8.x but should be "Contains \Drupal…" for next time.

Moving to 7.x for the backport.

thedavidmeister’s picture

Issue summary: View changes
Issue tags: +Novice

Making a backport is a novice task i think.

kirby14’s picture

I 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.

thedavidmeister’s picture

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.

hmmm, 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.

kirby14’s picture

hmmm, 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.

I 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.

thedavidmeister’s picture

great, thanks!

kirby14’s picture

FileSize
4.18 KB

Ok, let's give this a shot.

kirby14’s picture

FileSize
4.63 KB

I 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.

kirby14’s picture

Status: Patch (to be ported) » Needs review

The last submitted patch, 13: 2058761-13.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2058761-14.patch, failed testing.

kirby14’s picture

FileSize
4.64 KB

Really sorry about spamming the log. I'll get the hang of this.

kirby14’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: 2058761-18.patch, failed testing.

kirby14’s picture

Status: Needs work » Needs review
FileSize
4.42 KB

Ok, 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.

kirby14’s picture

FileSize
4.42 KB

Last 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?

thedavidmeister’s picture

You 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.

mgifford’s picture

@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.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

#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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2058761-22.patch, failed testing.

Status: Needs work » Needs review

dcam queued 22: 2058761-22.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Yet another random test failure.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2058761-22.patch, failed testing.

Status: Needs work » Needs review

mgifford queued 22: 2058761-22.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2058761-22.patch, failed testing.

Status: Needs work » Needs review

dcam queued 22: 2058761-22.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2058761-22.patch, failed testing.

Status: Needs work » Needs review

mgifford queued 22: 2058761-22.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2058761-22.patch, failed testing.

Status: Needs work » Needs review

dcam queued 22: 2058761-22.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 22: 2058761-22.patch, failed testing.

Status: Needs work » Needs review

mgifford queued 22: 2058761-22.patch for re-testing.

dcam’s picture

Status: Needs review » Reviewed & tested by the community
Fabianx’s picture

I agree with RTBC for that.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 7.x - thanks!

Nice cleanup work on those tests, too.

  • David_Rothstein committed bc7940a on 7.x
    Issue #2058761 by kirby14, thedavidmeister: PHP notice when #attributes...

Status: Fixed » Closed (fixed)

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