I found this warning because of a bug report on the field_group issue queue. When visiting the field settings / display settings from a content type. A strict warning is triggered.

Strict warning: Only variables should be passed by reference en field_ui_table_pre_render()

Files: 
CommentFileSizeAuthor
#10 1517654-10.patch678 bytessheise
PASSED: [[SimpleTest]]: [MySQL] 39,113 pass(es).
[ View ]
#1 strict_warning-1517654-variables_reference.patch698 byteszuuperman
PASSED: [[SimpleTest]]: [MySQL] 36,059 pass(es).
[ View ]

Comments

StatusFileSize
new698 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,059 pass(es).
[ View ]

Included is a patch

Status:Active» Needs review

Category:task» bug

Status:Needs review» Reviewed & tested by the community
Issue tags:+Novice

Trivial fix, removes about 10 PHP warnings on the field UI pages.

Status:Reviewed & tested by the community» Needs review
Issue tags:+needs backport to D7

Is this covered by the existing Field UI tests when running them locally with E_STRICT, the test bots currently don't have that set so it wouldn't show up as a branch failure, but it'd be good to know whether we actually have the test coverage or not. See also #1211914: E_STRICT on test bots.

Status:Needs review» Reviewed & tested by the community

All this patch is doing, is creating a local variable that can be then be passed by reference into current(). Do we really want tests for this?

If you are referring to creating an independent issue that looks at the test coverage in this area, then yes these are needed. In D7, the form only gets touched by the menu and upgrade tests afaick and probably is not in a state that would trigger the warnings. I ran these tests locally without any notices.

So not to stall a trivial bug fix that is effecting live sites, moving back to RTBTC.

Status:Reviewed & tested by the community» Needs review

All this patch is doing, is creating a local variable that can be then be passed by reference into current(). Do we really want tests for this?

No, what we want, is to know whether there are tests that would trigger the bug that the patch fixes, that's not the same as writing tests specifically to check the E_STRICT issue. So, if those tests exist, then that's great, if not, I'd be fine with a follow-up issue rather than making this patch depend on adding them, but I'm not aware of one existing yet.

Status:Needs review» Reviewed & tested by the community

No, when running the Field UI tests locally with E_STRICT no errors show up. The problem isn't reproducible with just Drupal Core, because there is no way a field could end up indented. We'd have to create a test module to do that. I remember a similar problem in a different issue (that I couldn't find), so maybe it's indeed worth creating such a test module in a follow-up.

As of #7 I believe this is RTBC now, but correct me, if you like ;)

Version:8.x-dev» 7.x-dev
Status:Reviewed & tested by the community» Patch (to be ported)

OK I opened #1593888: Tests for indentation in Field UI. Until #1211914: E_STRICT on test bots is done we can't add coverage that would actually catch this from the bot.

Committed/pushed to 8.x, moving to 7.x for backport.

Status:Patch (to be ported)» Needs review
StatusFileSize
new678 bytes
PASSED: [[SimpleTest]]: [MySQL] 39,113 pass(es).
[ View ]

Here's the backported D7 patch.

Status:Needs review» Reviewed & tested by the community

Looks good :) Lets get this in.

Prior to the patch, I counted 42 individual warnings on one field UI screen!

Status:Reviewed & tested by the community» Fixed

There's an interesting discussion about this topic going on over at #1572394: Language detection by domain only works on port 80. Neither I nor other people could reproduce any E_STRICT problems with current(). (If you replace it with reset(), end(), prev(), next(), etc, then the E_STRICT warnings appear, which makes sense because those functions actually alter the passed-in array but current() doesn't.)

Curious if anyone knows why this is reproducible by some people rather than others - could it depend on PHP version?

In any case, the patch looks fine either way, so I committed it to 7.x. Thanks!

http://drupalcode.org/project/drupal.git/commit/e2bcdbc

Thanks David, good to see you using your newly acquired status :)

FYI:

Linux www.example.com 2.6.18-028stab085.3-ent #1 SMP Mon Mar 21 19:57:43 MSK 2011 i686
PHP Version 5.2.17
error_reporting 6143

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