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

Closed
wants to merge 3 commits into
from

Projects

None yet

5 participants

Contributor

...dable or is a directory.

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

Member

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 commented Oct 15, 2013

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

Member

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

Contributor

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 @.

Member

@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.

Member

@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)

Member

@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.

Member

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.

Member

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.

Contributor

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.

Member

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).

Member

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.

Contributor

@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.

@lucascorbeaux lucascorbeaux Testing for include failure and empty buffer instead of file existence.
Slightly changed exception message and hide warnings in tests.
d55a594
Contributor

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 weierophinney and 1 other commented on an outdated diff Oct 23, 2013
library/Zend/View/Exception/UnexpectedValueException.php
+<?php
+/**
+ * Zend Framework (http://framework.zend.com/)
+ *
+ * @link http://github.com/zendframework/zf2 for the canonical source repository
+ * @copyright Copyright (c) 2005-2013 Zend Technologies USA Inc. (http://www.zend.com)
+ * @license http://framework.zend.com/license/new-bsd New BSD License
+ */
+
+namespace Zend\View\Exception;
+
+/**
+ * Unexpected value exception
+ */
+class UnexpectedValueException
+ extends \UnexpectedValueException
weierophinney
weierophinney Oct 23, 2013 Owner

Move this up to the previous line.

lucascorbeaux
lucascorbeaux Oct 24, 2013 Contributor

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
weierophinney Oct 24, 2013 Owner

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

@weierophinney weierophinney commented on an outdated diff Oct 23, 2013
tests/ZendTest/View/PhpRendererTest.php
+ 'not-exists' => '/does/not/exists',
+ ));
+
+ $this->renderer->setResolver($resolver);
+ @$this->renderer->render('not-exists');
+ }
+
+ public function testRendererRaisesExceptionIfResolvedTemplateIsDirectory()
+ {
+ $this->setExpectedException('Zend\View\Exception\UnexpectedValueException', 'file include failed');
+ $resolver = new TemplateMapResolver(array(
+ 'is-directory' => '.',
+ ));
+
+ $this->renderer->setResolver($resolver);
+ @$this->renderer->render('is-directory');
weierophinney
weierophinney Oct 23, 2013 Owner

I'd wrap these @-silenced calls to be more explicit in what you're testing:

set_error_handler(function ($errno, $errstr) { return true; }, E_NOTICE|E_WARNING);
try {
    $this->renderer->render('is-directory');
    $caught = false;
} catch (\Exception $e) {
    $caught = $e;
}
restore_error_handler();
$this->assertInstanceOf('Zend\View\Exception\UnexpectedValueException', $caught);
$this->assertContains('file include failed', $caught->getMessage());

You can then also remove the setExpectedException() calls, obviously. The above approach ensures that the notice and/or warnings are ignored by PHPUnit, and then allows us to test for the exception.

Owner

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

@lucascorbeaux lucascorbeaux Fix a code standard issue in the UnexpectedValueException, change the…
… testing strategy to avoid the use of the @ operator.
6475df7
Contributor

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

@weierophinney weierophinney added a commit that referenced this pull request Oct 24, 2013
@weierophinney weierophinney [#5266] implements
- "implements" stays on same line as declaration, interfaces on one line per
aded724
@weierophinney weierophinney added a commit that referenced this pull request Oct 24, 2013
@weierophinney weierophinney Merge branch 'feature/5266' into develop
Close #5266
Fixes #5258
5ea55e0
Owner

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

@stefanotorresi stefanotorresi referenced this pull request Oct 25, 2013
Merged

CS fix #5266 #5348

Member

Note: current implementation will:

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

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

Member

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

Contributor

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.

Member

@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).

@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#5266 from lucascorbeau…
…x/hotfix/5258

Throw an exception in PhpRenderer when the resolved file path is not rea...
24bb41d
@weierophinney weierophinney added a commit to zendframework/zend-view that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#5266] implements
- "implements" stays on same line as declaration, interfaces on one line per
31f87be
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment