Hi,
Since there appears to have been a little bit of trivia around fixing these phpcs warnings, I'll open a thread instead.
Both in javascript and PHP there are various keywords that can be used as if they are functions. In my opinion this is a misuse of the language and only causes confusion.
I'm referring to code like this (both javascript and php):
delete( mw.legacy );
new( mw.Title );
typeof( mw );
echo( $foo . $bar );
print( $foo . $bar );
return( $foo . $bar );
… and, wait for it..
require_once( $foo . $bar );
I think most experienced javascript developers know by now that using "delete" or "new" like it is a function is just silly and looks like you don't know what you're doing.
To give a bit of background, here's why these work at all (they aren't implemented both keywords and functions, just keywords). Though I'm sure the implementation details differ between PHP and javascript, the end result is the same: Keywords are given expressions which are then evaluated and the result is used as value. Since expressions can be wrapped in parenthesis for readability (or logic grouping), and since whitespace is insignificant to the interpreter, it is possible to do `return("test")`, which really is just `return ("test")` and eventually `return "test"`.
I'm obviously biased, but I think the same goes for "require_once" (and "include", "require" etc.). Right now this is causing quite a few warnings in our php-checkstyle report.
I didn't disable that rule because it appears (using our code base as status quo) that we do want this. There's 0 warnings I could find in our code base that violate this, except for when the keyword is include|require(_once)?
The check style sniffer does not (and imho should not) have a separate rule per keyword. Either you use constructs like this, or you don't.
But let's not have some weird exception just because someone didn't understand it[1] and we all copied it and want to keep it for no rational reason.
Because that would mean we have to either hack the sniffer to exclude this somehow, or we need to disable the rule, thus not catching the ones we do use.
See pending change in gerrit that does a quick pass of (most of) these in mediawiki/core:
https://gerrit.wikimedia.org/r/62753
-- Krinkle
[1] Or whatever the reason is the author originally wrote it like this. Perhaps PHP was different back then, or perhaps there was a different coding style.
But let's not have some weird exception just because someone didn't understand it[1] and we all copied it and want to keep it for no rational reason.
I think its probably because Die, List, Array, and Exit are also keywords but require the ().
It also makes sense when looking at language constructs like If, ElseIf, While, For, ForEach where although not strictly keywords I view as being the same for the purposes of this argument.
~Matt Walker Wikimedia Foundation Fundraising Technology Team
On Wed, May 8, 2013 at 5:26 PM, Krinkle krinklemail@gmail.com wrote:
Hi,
Since there appears to have been a little bit of trivia around fixing these phpcs warnings, I'll open a thread instead.
Both in javascript and PHP there are various keywords that can be used as if they are functions. In my opinion this is a misuse of the language and only causes confusion.
I'm referring to code like this (both javascript and php):
delete( mw.legacy ); new( mw.Title ); typeof( mw ); echo( $foo . $bar ); print( $foo . $bar ); return( $foo . $bar );… and, wait for it..
require_once( $foo . $bar );I think most experienced javascript developers know by now that using "delete" or "new" like it is a function is just silly and looks like you don't know what you're doing.
To give a bit of background, here's why these work at all (they aren't implemented both keywords and functions, just keywords). Though I'm sure the implementation details differ between PHP and javascript, the end result is the same: Keywords are given expressions which are then evaluated and the result is used as value. Since expressions can be wrapped in parenthesis for readability (or logic grouping), and since whitespace is insignificant to the interpreter, it is possible to do `return("test")`, which really is just `return ("test")` and eventually `return "test"`.
I'm obviously biased, but I think the same goes for "require_once" (and "include", "require" etc.). Right now this is causing quite a few warnings in our php-checkstyle report.
I didn't disable that rule because it appears (using our code base as status quo) that we do want this. There's 0 warnings I could find in our code base that violate this, except for when the keyword is include|require(_once)?
The check style sniffer does not (and imho should not) have a separate rule per keyword. Either you use constructs like this, or you don't.
But let's not have some weird exception just because someone didn't understand it[1] and we all copied it and want to keep it for no rational reason.
Because that would mean we have to either hack the sniffer to exclude this somehow, or we need to disable the rule, thus not catching the ones we do use.
See pending change in gerrit that does a quick pass of (most of) these in mediawiki/core:
https://gerrit.wikimedia.org/r/62753
-- Krinkle
[1] Or whatever the reason is the author originally wrote it like this. Perhaps PHP was different back then, or perhaps there was a different coding style.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 2013-05-08 9:26 PM, "Krinkle" krinklemail@gmail.com wrote:
Hi,
Since there appears to have been a little bit of trivia
Hi,
Since there appears to have been a little bit of trivia around fixing these phpcs warnings, I'll open a thread instead.
Both in javascript and PHP there are various keywords that can be used as if they are functions. In my opinion this is a misuse of the language and only causes confusion.
I'm referring to code like this (both javascript and php):
delete( mw.legacy ); new( mw.Title ); typeof( mw ); echo( $foo . $bar ); print( $foo . $bar ); return( $foo . $bar );… and, wait for it..
require_once( $foo . $bar );I think most experienced javascript developers know by now that using "delete" or "new" like it is a function is just silly and looks like you don't know what you're doing.
To give a bit of background, here's why these work at all (they aren't implemented both keywords and functions, just keywords). Though I'm sure the implementation details differ between PHP and javascript, the end result is the same: Keywords are given expressions which are then evaluated and the result is used as value. Since expressions can be wrapped in parenthesis for readability (or logic grouping), and since whitespace is insignificant to the interpreter, it is possible to do `return("test")`, which really is just `return ("test")` and eventually `return "test"`.
I'm obviously biased, but I think the same goes for "require_once" (and "include", "require" etc.). Right now this is causing quite a few warnings in our php-checkstyle report.
I didn't disable that rule because it appears (using our code base as status quo) that we do want this. There's 0 warnings I could find in our code base that violate this, except for when the keyword is include|require(_once)?
The check style sniffer does not (and imho should not) have a separate rule per keyword. Either you use constructs like this, or you don't.
But let's not have some weird exception just because someone didn't understand it[1] and we all copied it and want to keep it for no rational reason.
Because that would mean we have to either hack the sniffer to exclude this somehow, or we need to disable the rule, thus not catching the ones we do use.
See pending change in gerrit that does a quick pass of (most of) these in mediawiki/core:
https://gerrit.wikimedia.org/r/62753
-- Krinkle
[1] Or whatever the reason is the author originally wrote it like this. Perhaps PHP was different back then, or perhaps there was a different coding style.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On Wed, May 8, 2013 at 8:26 PM, Krinkle krinklemail@gmail.com wrote:
delete( mw.legacy ); new( mw.Title ); typeof( mw ); echo( $foo . $bar ); print( $foo . $bar ); return( $foo . $bar );… and, wait for it..
require_once( $foo . $bar );
I mostly agree. However, I must admit there are *some* cases where the parentheses are appropriate. However, in those situations, they should not be used as a function call operator. In other words, rather than:
return( $foo . $bar );
Sometimes (and only sometimes) it might be appropriate to do
return ( $foo . $bar );
In other words, the parentheses are being used to group the expression similar to how it's used in mathematical expressions, but not as a function call operator.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
we all copied it...
I learned the idiom require_once( 'path.php' ); from
* My wiki's LocalSettings.php , generated by includes/installer/LocalSettingsGenerator.php * The LocalSettings.php of labs instances, in puppet labs-localsettings * The installation instructions for every extension on mediawiki.org
All should change.
php.net has a comment agreeing with this change: "it will prevent your peers from giving you a hard time and a trivial conversation about what require really is" :-)
On Wed, May 8, 2013 at 5:26 PM, Krinkle krinklemail@gmail.com wrote:
Hi,
Since there appears to have been a little bit of trivia around fixing these phpcs warnings, I'll open a thread instead.
Both in javascript and PHP there are various keywords that can be used as if they are functions. In my opinion this is a misuse of the language and only causes confusion.
I'm referring to code like this (both javascript and php):
delete( mw.legacy ); new( mw.Title ); typeof( mw ); echo( $foo . $bar ); print( $foo . $bar ); return( $foo . $bar );… and, wait for it..
require_once( $foo . $bar );I think most experienced javascript developers know by now that using "delete" or "new" like it is a function is just silly and looks like you don't know what you're doing.
To give a bit of background, here's why these work at all (they aren't implemented both keywords and functions, just keywords). Though I'm sure the implementation details differ between PHP and javascript, the end result is the same: Keywords are given expressions which are then evaluated and the result is used as value. Since expressions can be wrapped in parenthesis for readability (or logic grouping), and since whitespace is insignificant to the interpreter, it is possible to do `return("test")`, which really is just `return ("test")` and eventually `return "test"`.
I'm obviously biased, but I think the same goes for "require_once" (and "include", "require" etc.). Right now this is causing quite a few warnings in our php-checkstyle report.
I didn't disable that rule because it appears (using our code base as status quo) that we do want this. There's 0 warnings I could find in our code base that violate this, except for when the keyword is include|require(_once)?
The check style sniffer does not (and imho should not) have a separate rule per keyword. Either you use constructs like this, or you don't.
But let's not have some weird exception just because someone didn't understand it[1] and we all copied it and want to keep it for no rational reason.
Because that would mean we have to either hack the sniffer to exclude this somehow, or we need to disable the rule, thus not catching the ones we do use.
See pending change in gerrit that does a quick pass of (most of) these in mediawiki/core:
https://gerrit.wikimedia.org/r/62753
-- Krinkle
[1] Or whatever the reason is the author originally wrote it like this. Perhaps PHP was different back then, or perhaps there was a different coding style.
Wikitech-l mailing list Wikitech-l@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/wikitech-l
On 09/05/13 10:26, Krinkle wrote:
I'm obviously biased, but I think the same goes for "require_once" (and "include", "require" etc.). Right now this is causing quite a few warnings in our php-checkstyle report.
include and require return a value, so they are more like functions than return or print. See e.g. ResourceLoader.php:
$this->register( include( "$IP/resources/Resources.php" ) );
You could compare them to pseudo-functions like empty and isset. I suppose you could write:
$this->register( include "$IP/resources/Resources.php" );
But that looks kind of weird to me.
-- Tim Starling
On Thu, May 9, 2013 at 9:22 PM, Tim Starling tstarling@wikimedia.orgwrote:
include and require return a value, so they are more like functions than return or print. See e.g. ResourceLoader.php:
$this->register( include( "$IP/resources/Resources.php" ) );
You could compare them to pseudo-functions like empty and isset. I suppose you could write:
$this->register( include "$IP/resources/Resources.php" );
But that looks kind of weird to me.
True, but you also have to be careful about that. As mentioned in the PHP docs, this can lead to weird operator precedence results. For example,
if ( include( "$IP/resources/Resources.php" ) === false ) {
will not work as expected. It looks like a quick check to see if the included file was successfully included, but PHP will first evaluate the === operator before the include statement.
*-- * *Tyler Romeo* Stevens Institute of Technology, Class of 2015 Major in Computer Science www.whizkidztech.com | tylerromeo@gmail.com
On May 10, 2013, at 3:22 AM, Tim Starling tstarling@wikimedia.org wrote:
On 09/05/13 10:26, Krinkle wrote:
I'm obviously biased, but I think the same goes for "require_once" (and "include", "require" etc.). Right now this is causing quite a few warnings in our php-checkstyle report.
include and require return a value, so they are more like functions than return or print. See e.g. ResourceLoader.php:
$this->register( include( "$IP/resources/Resources.php" ) );
You could compare them to pseudo-functions like empty and isset. I suppose you could write:
$this->register( include "$IP/resources/Resources.php" );
But that looks kind of weird to me.
-- Tim Starling
It might take some getting used to, but I don't think it looks off.
It is similar to the "new" keyword, which also returns value.
$masterdb->add( new SlaveDB() );
-- Krinkle
wikitech-l@lists.wikimedia.org