Page MenuHomePhabricator

Allow extensions to define additional command line parameters to "update.php"
Closed, ResolvedPublic

Description

https://gerrit.wikimedia.org/r/c/mediawiki/core/+/450097 made it so that many command line scripts error out if unknown parameters are provided. While this is a good thing in general, SMW relies on the ability for users to pass additional parameters to update.php script: https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/3507

An extension mechanism seems to be in order.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 16 2019, 9:00 AM
Kghbln added a subscriber: Kghbln.
Kghbln updated the task description. (Show Details)Jan 16 2019, 5:24 PM
BPirkle added a subscriber: BPirkle.Feb 6 2019, 8:47 PM

Per discussion with @tstarling , and consistent with discussion on the GitHub issue, I plan to add a hook allowing extensions to manipulate parameters to maintenance scripts. I'm now working out what the parameters to that hook should be, and how extensions should be allowed to manipulate these values.

Quick terminology review, because the naming gets a little confusing in the Maintenance class:

  • params: registry of named values passed to the script
  • arg list: registry of positional values passed to the script
  • options: passed param values
  • args: passed positional values

In the command "mwscript somescript.php --foo=bar baz"

  • foo is a param
  • bar is the option value of the option for param foo
  • baz is the arg value at index 0 in the arg list

This task was spawned by the need to add a "param" (--skip-optimize). I could simply add a hook allowing extensions to add parameters, and solve the immediate issue. However, just to be sure that's a sufficient long-term solution:

  • could there ever be a situation where an extension would need to add something to the arg list (perhaps a required arg whose absence will cause failure for the extension)?
  • could there ever be a situation where an extension would need to delete a param or arg? The Maintenance class offers a protected deleteOption function, but I don't see it called in my code base, so I'm not sure if that ever actually happens.
  • could there ever be a situation where an extension would need to modify a param or arg (perhaps making something required that was otherwise optional)?

Change 490531 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Allow extensions to add options and arguments to maintenance scripts

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

Nikerabbit moved this task from Backlog to Needing on the User-Nikerabbit board.Mar 5 2019, 8:15 AM

Change 490531 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Allow extensions to add options and arguments to maintenance scripts

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

Change 498296 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Allow extensions to add params to the update.php maintenance script

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

Change 498296 merged by jenkins-bot:
[mediawiki/core@master] Allow extensions to add params to the update.php maintenance script

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

Kghbln renamed this task from Allow extensions to define additional command line parameters to update.php to Allow extensions to define additional command line parameters to "update.php".EditedMar 31 2019, 4:46 PM
Kghbln closed this task as Resolved.
Kghbln triaged this task as High priority.
Kghbln removed a project: Patch-For-Review.

@BPirkle I understand that this is done now. :) Thanks a lot for your effort!

Change 490531 abandoned by Krinkle:
Allow extensions to add options and arguments to maintenance scripts

Reason:
Other patch landed.

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

TheDJ added a subscriber: TheDJ.EditedJul 10 2019, 9:46 AM

I note that this introduced php 7 syntax into the update.php script.
This means that if you use php56 and try to run update.php and accidentally you use php56 as a command line default etc. the version check for php won't help you in giving useful feedback as you will receive a syntax error.

A user reported in MediaWiki-General today with this exact situation (using php 7.1 but his webhoster had php cli symlinked to to php56).

I note that this introduced php 7 syntax into the update.php script.
This means that if you use php56 and try to run update.php and accidentally you use php56 as a command line default etc. the version check for php won't help you in giving useful feedback as you will receive a syntax error.

Do you think we should include documentation for this in the .1 release? Or should we figure out a fix so that the php version checks work?

Kghbln added a comment.EditedJul 10 2019, 4:11 PM

I note that this introduced php 7 syntax into the update.php script.

Why not? This was only backported from master to REL1_33 which has a minimum requirement of PHP 7.0. Though the situation the user encountered is indeed painful, admittedly I had a similar situation, I do not think that this patch has issues?

EDIT:

the version check for php won't help you in giving useful feedback as you will receive a syntax error.

Ah, so this broke the version check?

TheDJ added a comment.EditedJul 10 2019, 8:41 PM

the version check for php won't help you in giving useful feedback as you will receive a syntax error.

Ah, so this broke the version check?

Correct. So while it is possible to use php7 syntax, this particular file being a first class entrypoint into mediawiki, possibly should not if it can avoid to do so, out of user friendliness in the error reporting.

I guess ideally, you'd want a 'bootstrap' maintenance runner, that branches out into an update script. That would allow for a very simple and very backwards compatible entrypoint, where you could have this version check, before getting into more complicated logic. Unfortunately our current maintenance architecture doesn't have such a thing.

Next best thing is use old php56 syntax and suppress the linter that tells you to use php7 syntax I guess ?

Change 522014 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Allow PHP version check to execute on older versions of PHP

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

Change 522014 merged by jenkins-bot:
[mediawiki/core@master] Allow PHP version check to execute on older versions of PHP

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

Correct. So while it is possible to use php7 syntax, this particular file being a first class entrypoint into mediawiki, possibly should not if it can avoid to do so, out of user friendliness in the error reporting.

I totally agree.

Change 522014 merged by jenkins-bot:

Cool, an improvement already in place. :) I guess this needs to be back-ported to REL1_33 and REL1_34 in case the latter already exists.

Change 522095 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Allow PHP version check to execute on older version of PHP

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

Change 522095 merged by jenkins-bot:
[mediawiki/core@master] Allow PHP version check to execute on older version of PHP

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

Change 523704 had a related patch set uploaded (by Kghbln; owner: BPirkle):
[mediawiki/core@REL1_33] Allow PHP version check to execute on older versions of PHP

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

Out of despair I have now cherry-picked the first change. Probable after this was merged the follow up commit can also be cherry-picked to avoid conflicts. Thanks to all.

Change 523704 merged by jenkins-bot:
[mediawiki/core@REL1_33] Allow PHP version check to execute on older versions of PHP

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

Change 523717 had a related patch set uploaded (by TheDJ; owner: BPirkle):
[mediawiki/core@REL1_33] Allow PHP version check to execute on older version of PHP

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

Change 523717 merged by jenkins-bot:
[mediawiki/core@REL1_33] Allow PHP version check to execute on older version of PHP

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

I am stuck here. Gerrit does not allow me to cherry pick the second follow up commit. :(

TheDJ added a comment.Jul 16 2019, 2:23 PM

because i already did so ;)

because i already did so ;)

Ah, I get it now, great!. You are the best. Admittedly I was not able to interpret the error message correctly. :| You are cool! Thanks!!!