Throw an exception in PhpRenderer when the resolved file path is not rea...

[lenore-corbeaux]

...dable or is a directory.

Proposition to fix the issue which is discussed in the #5258.

[EvanDotPro]

Continuing the discussion from #5258, I don't see why this is necessary, honestly. Doesn't PHP already emit a warning for this?

Not readable (in this case, it's actually less ambiguous than the proposed exception message, IMO):

PHP Warning:  include(): Failed opening 'X' for inclusion (include_path=...) failed to open stream: Permission denied in Y on line Z

Directory (I agree this one could be confusing, but it's also quite a strange edge-case where you'd have a template map pointed at a directory by accident):

Warning: include(): Failed opening 'X' for inclusion (include_path=...) in Y on line Z

Keep in mind, to be able to throw this exception, we are adding extra stat calls in the renderer which will impact all requests to all standard ZF2 MVC applications. In the big picture, the performance issue is negligible, but I just don't see this adding enough value to justify it, personally.

That said, I haven't found myself spending a bunch of time debugging a template map that's pointed at a directory instead of a view script, so take that for what it's worth.

[Skpd]

It is hard to catch warnings in production environment. Maybe checking view scripts should be configurable?

[Thinkscape]

@Skpd That's why you use diagnostics in production to catch those types of errors.

[lenore-corbeaux]

I understand that extra stat calls, even if they are few, are a performance issue that shouldn't be neglected. But I strongly believe that a framework should be consistent and clearly indicates that something is wrong, which is not the case here.

What about testing the include's return value ? If include returns false then there is a problem, and then we can throw an exception, without performance costs at all.

That way, the fix is much more difficult to test because we have to handle a PHP warning… or mute him with a very dirty @.

[EvanDotPro]

@lucascorbeaux I think interpreting the include return value is a decent solution. Technically this could cause confusion if someone was returning false explicitly from a view script (wtf?), but we could even protect against that by making only triggering the exception if include returns false AND the output buffer is empty.

[Thinkscape]

@EvanDotPro I can imagine this being the case with view helpers, i.e. somebody having the following view script:

<?php
return $this->prepareMyResultsSomehow($this->data);

Now the custom view helper might return void for some reason.

Also - what if there's an empty view script ? (or due to processing the view script the result is empty, i.e. processing some data, empty collection and results in empty result)

[EvanDotPro]

@Thinkscape What is the use-case for them having the return statement like that in an otherwise empty view script? You're correct that a user would be affected in that case, I just can't see why someone might be doing that.

[EvanDotPro]

The reason I say that is, the PhpRenderer explicitly does not care about (nor does it store, assign, or make available) the return value from the view script in any way, so if anyone is doing that currently, I would imagine it is in vain and not accomplishing whatever they're expecting it is.

[Thinkscape]

I agree, I just predict that some people might have something like the above. It's not very smart, usually it doesn't do anything (i.e. as far as "templates" are considered) and should be discarded. Of course, it's an edge case so if we're gaining more than we're risking with this "1 in a 10000" scenario, then let's do it.

What you'd call "empty view script" is just a view script. Forget html for a moment, but when using view scripts for xml, json, rpc messages and such, you'd be forming some result which might not contain html fragments. So basically, in many cases I'd have view scripts which are just php scripts that prepare and produce result as per the rendering process (not to confuse with *ViewModel and strategies, which usually don't use rendering phase). One example could be an api server, which has a view script for building json results, so you'd throw a view model at it containing data, then have a rendering phase (with view script) which sanitizes/obfuscates/mangles/transforms the data so it's safe to transmit to the user.

[lenore-corbeaux]

I haven't expected the "return false" case. Even if I don't see which use case can produce this it's a possible side effect...

A bit tricky, but what about changing then restoring the error handler to handle the include's warning and throws exception if raised ? I really have no idea of the exact performance cost of swapping error handler, but I think it's far less than stat calls.

[Thinkscape]

but I think it's far less than stat calls.

I don't think so. Stat calls are cached, swapping is a lot of function calls and internal php stuff that's not very well optimized (same as @ operator).

[EvanDotPro]

Of course, it's an edge case so if we're gaining more than we're risking with this "1 in a 10000" scenario, then let's do it.

This sums up my opinion.

A bit tricky, but what about changing then restoring the error handler to handle the include's warning and throws exception if raised ?

Yeah, this is another option I had thought of intially too, but I didn't suggest it because it would need to make sure it's the warning we're interested in, or else we'd be escalating any warning thrown by a view script to an exception, which I think could potentially affect a lot more people than interpreting the include return value. Overall, that approach just feels a little dirty to me.

[lenore-corbeaux]

@EvanDotPro Yes, error handling to prevent the include's warning is quite dirty, and according to a very quick bench on my SSD powered laptop @Thinkscape is right about performance, error handler swapping is slower than stats call.

I'll sent ASAP a PR with a test on the include return value, but I will be forced to overload the error handler in the unit tests, to avoid the test suite emitting warnings.

[lenore-corbeaux]

Here is the PR. Note that the case where the view returns false and buffer is not empty has been implemented but not tested. Seems quite unlikely to happen and difficult to test (relying on file system...).

Somebody have a better suggestion than the ugly @ in the test cases to avoid PHP emitting warnings ? Seems less tricky to me than changing error handler and replacing setExpectedException with try - fail / catch - restore handler.

I also change the exception message.

[weierophinney]

Move this up to the previous line.

[lenore-corbeaux]

You mean move the extends in the same line than the class declaration ? Because the others Zend\View exceptions are all formatted the same way.

[weierophinney]

Yes, move it up. The other ones need to be fixed. :)

[weierophinney]

@lucascorbeaux Looking good -- some tweaks to testing strategies, and this is ready.

[lenore-corbeaux]

Thanks for your review ! Here are the fixes, I refactored a bit the test with a data provider to avoid code duplication.

[weierophinney]

Merged to develop for release with 2.3.0, as it's a slight behavior change.

[Thinkscape]

Note: current implementation will:

1. Throw E_WARNING, and then
2. Throw exception.

I'm not sure that's exactly what was expected...

[Thinkscape]

This means, that this statement is invalid:

if ($includeReturn === false && empty($this->__content)) {

The E_WARNING thrown for non-existing file will be captured by the output buffering, so $this->__content can never be empty unless error reporting or display_errors is explicitly disabled.

Here's a quick test: http://3v4l.org/P9UFL

[lenore-corbeaux]

Good catch ! It seems that the unit test error handling and my local configuration hides this behavior so I didn't noticed it. Even if the warning emitted before an exception was expected, it was obviously not wanted that the fix works only in environments with errors disabled...

The only fix I see is to only check the false return value, with the side effect that a view returning false will throw an exception.

[Thinkscape]

@lucascorbeaux We've been discussing it on the other ticket, take a look at the discussion, remarks from me and @EvanDotPro.

Basically, there are at least 2 possible scenarios:

1. The file is missing or is not readable (which throws E_WARNING and causes include to return false)
2. The included file produces no output ($output === null)

I believe these should be separated and handled accordingly.

For 1. I believe we've reached a conclusion before, that it's best to use is_readable() test because it is independent of the structure and implicit errors of the php script that is being included.

For 2. it's best to just perform a simple empty($output) test.

I also think those scenarios should produce different exceptions. A missing or unreadable file should be explicitly pronounced, so developer can quickly verify its existence and permissions. An empty view script could mean an error in the file itself (i.e. it is really empty, or there's some code error inside it that is suppressed or something else that is unexpected).