Page MenuHomePhabricator

Use short array syntax on AdminLinks.alias.php
Closed, DeclinedPublic

Description

I made https://gerrit.wikimedia.org/r/#/c/374780/ to convert that page to use short array syntax but jenkins failed. Made it depend on the patch to convert the extension to use extension registration, but it failed too. I am lost as to why "short array syntax is not allowed" there. Thanks.

Event Timeline

Change 374780 had a related patch set uploaded (by MarcoAurelio; owner: MarcoAurelio):
[mediawiki/extensions/AdminLinks@master] Use short array syntax on AdminLinks.alias.php

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

Generic.Arrays.DisallowShortArraySyntax.Found

This is what jenkins-bot says. Still can't find where this is disallowed.

See phpcs.xml which disallows the short array syntax in favor of PHP 5.3 compatibility. If you want this patch to pass, remove lines 7 to 9.

Actually, the whole file can be deleted, as extension registration enforces the other rule, and PHP 5.3 compatibility is no longer a thing.

@Mainframe98 Thank you. So I think we can delete the file and make the patch depend on the extension registration migration one. I'll try that. Otherwise @Urbanecm says to remove lines 7 to 9 as you also suggested.

@Mainframe98 I've deleted the file and now the test passes. I however see that other extensions keep the file.

Another question I see is that rEADL's composer.json file misses --exclude node_modules as some other extensions do. Shall we add it there too?

MarcoAurelio renamed this task from "Short array syntax not allowed" on AdminLinks.alias.php to Use short array syntax on AdminLinks.alias.php.Aug 30 2017, 11:34 AM
MarcoAurelio claimed this task.

I'm not entirely sure on how --exclude node_modules actually relates to anything; it appears to be used for JavaScript libraries and JSON linting. While it does have JSON files, I am not aware of any JavaScript files in this repository, and thus I'm inclined to say it is unnecessary.

@Legoktm What is your opinion with regards to keeping or deleting the phpcs.xml file in this case and, if we have to keep it, how to make it work with short array? Thanks.

Don't delete the phpcs.xml, it's necessary to run tests properly. To allow them, you just need to delete the <rule> line that disallows them. But more importantly, you should check with the maintainer of the extension, @Yaron_Koren, that it's OK to drop PHP 5.3 support.

Thanks for asking. Dropping support for PHP 5.3 means dropping support for MediaWiki versions below 1.27. I would rather not drop support for these quite yet, especially MW 1.26, which was in use until about a year ago. So I would say no, for now.

Support for MediaWiki <1.29 would be dropped with T173058 since the proposed patch uses manifest version 2, only supported in 1.29+.

Feel free to let @Reception123 know on the patch that it should use manifest version 1 instead to retain compatibility with <1.29. The patch is not merged yet after all, so there's still time to do something about this.

@Mainframe98 Are there any parameters currently incompatible with manifest version 1? If so please tell me which here, or on the patch and I will make the edit.

Only one: the config parameter. Thing is, AdminLinks doesn't have that, so all you have to do is change the manifest version to 1, and update AdminLinks.php to have the error message say 1.25+ instead of 1.29+

Ok, then. @Yaron_Koren Changed to manifest version 1.

Change 374780 abandoned by MarcoAurelio:
Use short array syntax on AdminLinks.alias.php

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

MarcoAurelio moved this task from working on to cabinet on the User-MarcoAurelio board.

Don't delete the phpcs.xml, it's necessary to run tests properly. To allow them, you just need to delete the <rule> line that disallows them. But more importantly, you should check with the maintainer of the extension, @Yaron_Koren, that it's OK to drop PHP 5.3 support.

Thank you for your help. I'll keep all of that in mind.