slimphp / PHP-View

A Simple PHP Renderer for Slim 3 & 4 (or any other PSR-7 project)
MIT License
264 stars 60 forks source link

the reserved $template key in data array #40

Closed ncou closed 4 years ago

ncou commented 6 years ago

Hi,

Nice piece of code. Juste 2 questions : 1) why do you keep a check on the "$template" key present in the data array, while in this variable is now protected because the function protectedIncludeScope use func_get_arg(0) so there is no risk to overwrite it with the extract() function.

I think this check is useless : https://github.com/slimphp/PHP-View/blob/master/src/PhpRenderer.php#L149

Am i missing something ????

2a) Why not use a require instead of an include ? there is already a check if the file exist, so using require() is pretty safe from my point of view.

2b) Are you sure there could have a throw exception/throwable from the extract() function ? are the try/catch really usefull ?

Thank you for your clarifications. Keep up the good work.

meetmatt commented 6 years ago

Hi,

  1. I agree with you, it seems like this check doesn't make sense any more after https://github.com/slimphp/PHP-View/commit/6bb302549e87531a87a6ee6972c94ab5403fd562

2a. I can only imagine a strange case when due to race condition the template file does no longer exist after it has been checked on L149 but before it's actually included. With require you will still get an exception in this case. But I'm sure there must have been a more simple reason, also it's possible that the check was added later, didn't check the actual commit log.

2b. try/catch wraps the required template, rather than extract, and is used to not allow template to render partially in case it throws an exception (e.g. somewhere in the middle of the included template you have a fatal error). As you may see in both catch blocks the output buffer gets cleaned before re-throwing the initial exception.

meetmatt commented 6 years ago

2b. Actually I found a case when extract will throw an exception: when the data contains "this" key. It will result in "Fatal error: Uncaught Error: Cannot re-assign $this".

akrabat commented 4 years ago

2a. Given that include and require are identical when the file exists, it makes no odds which is used. include was the original author's preference.

akrabat commented 4 years ago
  1. It turns out that func_get_arg(0) picks up the value of the first parameter at the point that it's called, not at the point it was passed into the function. See https://3v4l.org/nGGOk

We could use extract($data, EXTR_SKIP);, which would silently ignore $data['template'], but I think that would result in confusion compared to an explicit error.