From #348448: Always report E_STRICT errors it looks like the test bots are currently on E_ALL.

webchick said she's very much not in favour of setting E_STRICT in D7 (which makes sense on balance), but having the test bots E_STRICT would help with PHP 5.4 compliance, and help avoiding committing regressions to D7.

Comments

catch’s picture

Category: task » feature
jrchamp’s picture

Are we happy with E_ALL | E_STRICT? It seems like -1 is slightly more future proof, but I understand that it's not intuitive if you don't already have a working knowledge of the PHP error levels.

catch’s picture

Hmm, I'd prefer E_ALL | E_STRICT just because it's more self-documenting.

jrchamp’s picture

That's fine. It's not like PHP error levels change regularly. How do we make just the test bots strict? (Is that a Drupal change or a separate test bot change?)

catch’s picture

It'll be a test bot change. Test bots can theoretically be managed individually, but in practice rfay is doing the bulk of that at the moment, and I believe there's a common AWS image or similar for it. Not sure exactly what would need to be done, but something like manually setting E_STRICT on each bot, then changing the bot documentation and/or image to reflect the change.

If I've got this wrong and the bots already have E_STRICT set then there's a bug somewhere.

catch’s picture

Bump, I'd still like to do this, I'll try to ping webchick to see if she still feels the same way about not doing it.

Damien Tournoud’s picture

+1 here too. As far as I know test bots are spawned by Puppet, so this should be relatively easy to do.

jrchamp’s picture

FWIW, E_ALL includes E_STRICT as of PHP 5.4.0

webchick’s picture

Hm. I'm trying to remember what 2011 webchick's concerns were around E_STRICT.

The only thing I can think is a worry that it would be akin to when we made Drupal E_ALL compliant, which was months of work and 5000K patches to do. Not the kind of thing you want to embark in during a "stable" release. (None of us in IRC at the time catch asked thought it would be remotely this bad, however.)

Another huge concern would be if changes to Drupal core to make it E_STRICT compliant would ripple out to required changes in contributed modules. We broke BC on a couple of major module in 7.14 so I'm a little gun-shy now. :(

In general though, I'm +1 to this change. We've already committed some PHP 5.4 fixes and it'd be lovely to make sure they continue to work. I think a sensible first step is to try flipping the switch on a one-off testbot and run it against D7 and see what breaks. jthorson offered to do this.

webchick’s picture

Status: Active » Reviewed & tested by the community

http://qa.staging.devdrupal.org/7.x-status is green! Hooray!

I guess there's no reason not to do this then. :)

chx’s picture

It's much easier to make code E_STRICT compatible than it was to make it E_ALL compatible. It also makes absolutely no sense. You can't do current(some_function()) because current wants a reference. While E_ALL has practical uses and actually discovers funny little bugs around the intersections of empty/isset I have yet to see a single bug discovered due to E_STRICT. On the other hand, I see more bloated code to be compatible with this newest nonsense PHP spun.

chx’s picture

Oh and I know we will go ahead nonetheless and probably we should due to PHP 5.4 making this idiocy approx mandatory but my opinion needed to be recorded.

jthorson’s picture

Just a note ... The test in #10 was run on scratchtestbot, but I don't know whether rfay turned on error reporting on this testbot when he did the production bots ... thus, we could still have some 'hidden' errors; especially since the type of issues flagged by E_STRICT won't necessarily affect the operation of the code.

It would probably be prudent to double-check on a single production bot before rolling the same change out to the rest of the testbots (and puppet instance).

webchick’s picture

http://drupal.org/node/1273722#comment-5415368 applied with git apply -R to the 7.x branch should test to see if it catches E_STRICT warnings.

Also, I accidentally found #1281866: Simpletest emits many E_STRICT warnings on PHP 5.3 while searching for that, which outlays a whole bunch of contributed modules' tests that are going to start failing when this is done.

Given that, it might make sense to schedule/announce at groups.drupal.org/core and/or possibly through the contributors mailing list.

chx’s picture

Status: Reviewed & tested by the community » Needs review

So what we have here is that for example getInfo needs to be a static method. Now, static was throwing a syntax error for PHP4. So a bunch of Drupal 6 contrib, most notably Views 6.x-2.x will not be testable if you make the bot E_STRICT. (Not that I have any clue about whether Views 6.x-2.x has any tests.)

catch’s picture

Status: Needs review » Reviewed & tested by the community

There's nothing to stop Views 6.x-2.x shipping with tests that depend on PHP 5.2, also I'm pretty sure it doesn't have any test coverage, but the same goes for any other module that's supporting PHP4.

tim.plunkett’s picture

Neither of the Views 6.x branches have tests.

jthorson’s picture

Project: Test driven development infrastructure » Drupal.org Testbots
Status: Reviewed & tested by the community » Active

Updating queue

jthorson’s picture

For core, Testbot 1823 is now set to E_ALL & ~E_DEPRECATED (on php 5.4), so that can be used to periodically test for E_STRICT notices without potentially affecting any D6 / D7 / Contrib results.

I've just kicked off a retest of the D8 on PHP 5.4 test with the new parameter.

isntall’s picture

Project: Drupal.org Testbots » DrupalCI: Test Runner
Issue summary: View changes
isntall’s picture

Project: DrupalCI: Test Runner » Drupal.org Testbots

shouldn't have moved this.

Mixologic’s picture

Status: Active » Closed (outdated)

#2771843: Ensure consistent PHP Error reporting is where we can look at this further in drupalci