Page MenuHomePhabricator

add a check for whitespace before leading <?php
Closed, ResolvedPublic

Description

today we had an outage caused by whitespace before the leading <?php in a file in wmf-config

there have been different suggestions on how to prevent that in the future:

< ori> we should just check for this very specific thing in scap
< ori> if filename.endswith('.php') and not file.startswith('<?php')

< Krenair> Let's make phpcs voting

< twentyafterfour> at deviantart we had a very strict commit hook that wouldn't accept any file with whitespace before the <?

Event Timeline

Dzahn created this task.Mar 12 2015, 6:24 PM
Dzahn raised the priority of this task from to Needs Triage.
Dzahn updated the task description. (Show Details)
Dzahn added a subscriber: Dzahn.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 12 2015, 6:24 PM
Krinkle added a subscriber: Krinkle.EditedMar 12 2015, 6:27 PM

See T46875: Jenkins: Create phpcs sniff to detect text before the first <?php tag.

This and various other errors are already caught by phpcs. Please ensure project(s) use phpcs and have it passing/voting.

See T46875: Jenkins: Create phpcs sniff to detect text before the first <?php tag.
This and various other errors are already caught by phpcs. Please ensure project(s) use phpcs and have it passing/voting.

Good luck with that in mediawiki-config :P

ori added a comment.Mar 12 2015, 6:32 PM

See T46875: Jenkins: Create phpcs sniff to detect text before the first <?php tag.
This and various other errors are already caught by phpcs. Please ensure project(s) use phpcs and have it passing/voting.

Straight-out syntax errors are caught by Jenkins too, but we still have the syntax check in scap as a last line of defense. Don't forget that in emergencies, we sometimes have to live-hack code and sync it without waiting on CI.

greg triaged this task as High priority.Mar 12 2015, 6:33 PM
greg added a subscriber: greg.
In T92531#1114010, @ori wrote:

See T46875: Jenkins: Create phpcs sniff to detect text before the first <?php tag.
This and various other errors are already caught by phpcs. Please ensure project(s) use phpcs and have it passing/voting.

Straight-out syntax errors are caught by Jenkins too, but we still have the syntax check in scap as a last line of defense. Don't forget that in emergencies, we sometimes have to live-hack code and sync it without waiting on CI.

We can (and should) run composer-test (or phplint/phpcs/phpunit individually) on tin before syncing config changes.

greg added a comment.Mar 12 2015, 7:49 PM

So T92534: scap's check_php_syntax() should check for text before '<?php' in PHP files is about the scap part (with a patch from Lego).

This is only about CI, yes? Bring any scap/deployment related discussion over to T92534 (or a new bug if needed).

I'll clarify the description if that ^^ is indeed true.

So T92534: scap's check_php_syntax() should check for text before '<?php' in PHP files is about the scap part (with a patch from Lego).
This is only about CI, yes? Bring any scap/deployment related discussion over to T92534 (or a new bug if needed).
I'll clarify the description if that ^^ is indeed true.

Sure. T92534 has already addressed the issue in scap, so let's focus this one on CI.

hashar lowered the priority of this task from High to Medium.Jun 5 2015, 12:25 PM
hashar added a subscriber: hashar.
hashar lowered the priority of this task from Medium to Low.Aug 25 2015, 1:12 PM
jayvdb added a subscriber: jayvdb.Jan 6 2016, 3:12 AM
Krinkle removed a subscriber: Krinkle.Feb 23 2016, 4:56 PM
greg added a comment.Sep 29 2016, 7:42 PM

This follow-up task from an incident report has not been updated recently. If it is no longer valid, please add a comment explaining why. If it is still valid, please prioritize it appropriately relative to your other work. If you have any questions, feel free to ask me (Greg Grossmeier).

Seb35 added a subscriber: Seb35.Sep 28 2017, 8:35 PM

I read this bug and I don’t understand what remains to be done [to close this task]:

  • T46875 about MediaWiki is now a standard PHPCS rule executed in CI;
  • T92534 in scap is now done;

Are there remaining parts to be checked? perhaps extensions/skins used by WMF?

(This task makes me thinking about another related bug about the closing tag T10493, perhaps it could be added a similar PHPCS rule, but that’s another issue.)

greg closed this task as Resolved.Sep 28 2017, 8:53 PM
greg claimed this task.

Thanks @Seb35 I think we're good here (afaict)!

Restricted Application added a project: User-greg. · View Herald TranscriptSep 28 2017, 8:53 PM