Side effects in PSR-1

[Greg Sherwood]

Hi all,

I've got one sniff left to write for the PSR-1 standard in PHP_CodeSniffer, but it's the most complicated one: 2.3 Side Effects

https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md#23-side-effects

Everything in this section is just warnings (SHOULD instead of MUST) but it's also the one that I think most developers will need help figuring out. The other rules are pretty basic, so I think it would be helpful if PHPCS was able to determine if your file has side effects or not.

So I'm trying to nail down what a side effect is. The standard helpfully defines this and includes a few examples, but I'd like it if people can check my logic.

Firstly, if your file contains a class, interface, trait (?), function or constant, you can't have any other code outside of these declarations unless they are:

- a declare() statement

- a NAMESPACE statement

- a USE statement

- an IF statement that only includes a class/function/etc declaration (as in the function_exists() example in the standard)

Now where it gets a little bit more complicated is for code like this:

$bar = false;

$foo = Class:getFoo();

if ($foo) {

  $bar = true;

}

Simply including the file will set a global variable $bar and another call $foo (in this case). Is that a side effect?

My opinion, based on a reading of the standard, is that this would not be allowed in a file that also declares functions/classes etc because it is the "execution of logic not directly related to declaring classes, functions, constants, etc".

Is my logic right here? Any other code examples that I should be aware of or that are slightly ambiguous that I should take into account?

Thanks for all the feedback so far on my other questions.

[Paul M. Jones]

Hi Greg -- just to you.

> I've got one sniff left to write for the PSR-1 standard in PHP_CodeSniffer, but it's the most complicated one: 2.3 Side Effects
> https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-1-basic-coding-standard.md#23-side-effects

That is an amazingly difficult thing to automate. Kudos for even trying.


-- pmj

[Carlos Campderrós]

Hi,

On Wed, Aug 8, 2012 at 4:38 AM, Greg Sherwood <gshe...@gmail.com> wrote:

Now where it gets a little bit more complicated is for code like this:

$bar = false;

$foo = Class:getFoo();

if ($foo) {

  $bar = true;

}

Simply including the file will set a global variable $bar and another call $foo (in this case). Is that a side effect?

My opinion, based on a reading of the standard, is that this would not be allowed in a file that also declares functions/classes etc because it is the "execution of logic not directly related to declaring classes, functions, constants, etc".

I agree with you and consider this code a side effect if the file where it is stored contains declarations of anything.

--
Si no puedes deslumbrar con tu sabiduría,
desconcierta con tus gilipolleces

[Larry Garfield]

I agree as well. Declaring global variables is a side-effect, and thus
is not allowed in a token-declaring file. (They shouldn't be allowed
period, but that's another story. <g>)

--Larry Garfield

[Greg Sherwood]

On Thursday, 9 August 2012 06:23:52 UTC+10, Larry Garfield wrote:

I agree as well.  Declaring global variables is a side-effect, and thus
is not allowed in a token-declaring file.  (They shouldn't be allowed
period, but that's another story. <g>)

Yeah sorry. I was just trying to come up with a quick example; not advocating for random global variables sitting around. 

[Greg Sherwood]

Thanks to everyone who commented. Please keep them coming.

In the meantime, I've pushed my first attempt at a sniff to detect files that both declare symbols and cause side effects. It's in the Github repo.

I'd appreciate it if people could test on their code to see if there are any other code blocks that need considering. The best way to do this is to clone the git repo and run PHPCS directly from there:

git clone git://github.com/squizlabs/PHP_CodeSniffer.git

cd PHP_CodeSniffer

php scripts/phpcs --standard=PSR1 --sniffs=PSR1.Files.SideEffects /path/to/code

The commands above clone the repo and run the PSR1 standard with just this new sniff so it is easy to identify the files causing this specific warning. If you'd like to also do a test run of the (now complete) PSR1 or PSR2 standards, you can use the commands:

php scripts/phpcs --standard=PSR1 /path/to/code

php scripts/phpcs --standard=PSR2 /path/to/code

Just as an example of what to expect, if you run it on the sample code in the PSR-1 standard:

 1: <?php

 2: // side effect: change ini settings

 3: ini_set('error_reporting', E_ALL);

 4:

 5: // side effect: loads a file

 6: include "file.php";

 7:

 8: // side effect: generates output

 9: echo "<html>\n";

10:

11: // declaration

12: function foo()

13: {

14:     // function body

15: }

You'll get:

--------------------------------------------------------------------------------

FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)

--------------------------------------------------------------------------------

 1 | WARNING | A file should declare new symbols (classes, functions,

   |         | constants, etc.) and cause no other side effects, or it should

   |         | execute logic with side effects, but should not do both. The

   |         | first symbol is defined on line 12 and the first side effect is

   |         | on line 3.

--------------------------------------------------------------------------------

If you're reporting issues, it would be great if you can also include the message you got so I can see what lines PHPCS thought the symbol and side effect were on.

All testing and feedback is greatly appreciated.

[Paul Dragoonis]

It looks good, i'll try this on ppi2, and let you know if there's any issues.
Paul.

> --
> You received this message because you are subscribed to the Google Groups
> "PHP Framework Interoperability Group" group.
> To post to this group, send email to php...@googlegroups.com.
> To unsubscribe from this group, send email to
> php-fig+u...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msg/php-fig/-/qrVubT8p9msJ.
>
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Steven Scott]

I am getting an error and not sure why with this sniff.  I have require_once a class in my file to extend.  When I remove the require, the sniff passes.  I need the require_once for the code to execute (I am not using an AutoLoader);

\lib\FOO.php
namespace lib;
class FOO implements \Iterator
{
}

\lib\Objects\Bar.php
namespace lib\Objects;
require_once(dirname(__FILE__) . '/../FOO.php');
class BAR extends FOO
{
}

FILE: A:\lib\Objects\BAR.php

--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 1 | WARNING | A file should declare new symbols (classes, functions,
   |         | constants, etc.) and cause no other side effects, or it should
   |         | execute logic with side effects, but should not do both. The

   |         | first symbol is defined on line 6 and the first side effect is
   |         | on line 4. (PSR1.Files.SideEffects.FoundWithSymbols)
--------------------------------------------------------------------------------

Time: 0 seconds, Memory: 2.50Mb

[Larry Garfield]

A require is a side-effect, in that it is an executable line of code
that may do more than just declare symbols. The sniffer is right in
this case.

Your options are

1) Use an autoloader. This is the better option.

2) Manually include the parent class first (Foo.php) before including
Bar.php.

Also, there's now a new PHP-FIG-CS mailing list for these questions.
Please use that one instead. :-)

--Larry Garfield

> --
> You received this message because you are subscribed to the Google
> Groups "PHP Framework Interoperability Group" group.
> To post to this group, send email to php...@googlegroups.com.
> To unsubscribe from this group, send email to
> php-fig+u...@googlegroups.com.
> To view this discussion on the web visit

> https://groups.google.com/d/msg/php-fig/-/h-DKN0-6UpUJ.