Page MenuHomePhabricator

sync-* and scap should only lint changed files from last deploy
Open, LowPublic

Details

Related Gerrit Patches:
mediawiki/vendor : master<?PHP -> <?php due to scap bug
mediawiki/vendor : wmf/1.27.0-wmf.11<?PHP -> <?php due to scap bug

Event Timeline

Reedy created this task.Jan 20 2016, 2:26 PM
Reedy raised the priority of this task from to Needs Triage.
Reedy updated the task description. (Show Details)
Reedy added subscribers: Aklapper, Reedy, StudiesWorld.

Change 265273 had a related patch set uploaded (by Reedy):
<?PHP -> <?php due to scap bug

https://gerrit.wikimedia.org/r/265273

Change 265273 merged by jenkins-bot:
<?PHP -> <?php due to scap bug

https://gerrit.wikimedia.org/r/265273

Change 265274 had a related patch set uploaded (by Reedy):
<?PHP -> <?php due to scap bug

https://gerrit.wikimedia.org/r/265274

Change 265274 merged by jenkins-bot:
<?PHP -> <?php due to scap bug

https://gerrit.wikimedia.org/r/265274

Reedy added a comment.Jan 20 2016, 2:51 PM

Note, these patches don't fix the issue in this bug with scap vs sync-dir behaviour. Just stops <?PHP breaking stuff when it's valid PHP etc

Reedy set Security to None.
bd808 added a subscriber: bd808.Jan 20 2016, 5:58 PM

scap seemingly allows <?PHP, but sync-dir doesn't?

scap only checks the files in the config directory for sanity and assumes that Jenkins checked other things so that's probably the difference there.

Reedy added a comment.Jan 20 2016, 7:43 PM

Part of me thinks scap should do it too... But it's extra time. In the grand scheme of how long scap takes now, it's probably not a great deal

But, I guess, fixing the wrong check in this case, is enough to fix this; as both <?PHP and <?php are valid, just our implemented checker was a bit naive (even if <?PHP isn't common)

bd808 added a comment.Jan 20 2016, 8:56 PM

Having scap lint every php file in the active branches is going to be slow and I honestly don't see the value in it. sync-file/sync-dir lint is more reasonable as they may be linting security patches or config files that haven't been through a linter in the CI tool chain.

demon added a subscriber: demon.Jan 20 2016, 9:03 PM

We should generate a list of changed files and only lint those.

greg edited projects, added scap2; removed Deployments.Feb 9 2016, 11:51 PM
greg renamed this task from sync-dir doesn't like <?PHP, but scap does? to sync-* and scap should only lint changed files from last deploy.Apr 4 2016, 9:01 PM
greg triaged this task as Low priority.
greg updated the task description. (Show Details)
mmodell edited projects, added Scap; removed scap2.Feb 10 2017, 6:22 PM