Improvements rendering PHP files

@SamMousa commented on Jul 9, 2018, 10:05 AM UTC:

Currently rendering PHP files is an exception with respect to other files in the View class:

if (isset($this->renderers[$ext])) {
                if (is_array($this->renderers[$ext]) || is_string($this->renderers[$ext])) {
                    $this->renderers[$ext] = Yii::createObject($this->renderers[$ext]);
                }
                /* @var $renderer ViewRenderer */
                $renderer = $this->renderers[$ext];
                $output = $renderer->render($this, $viewFile, $params);
            } else {
                $output = $this->renderPhpFile($viewFile, $params);
            }

1. I propose removing the exception and implementing a PhpRenderer and having a default renderer in the View class.

Currently rendering of PHP files happens inside the object context, this means that the View object itself is available via $this. This makes several tools unhappy, since you're calling $this outside of an objects' context, as far as they can tell.
Since you need to use something like assertions or PHPDoc to type hint the parameters anyway, I feel there could be better ways to make the View object available, for example by naming it $view instead of $this.

2. Implement PHP file rendering by including the file inside a closure that has a static scope:

$renderer = function() use ($file, $params) {
  extract($params, EXTR_OVERWRITE);
  require $file;
};
$_obInitialLevel_ = ob_get_level();
ob_start();
ob_implicit_flush(false);
try {
    $renderer->bindTo(null)()
    return ob_get_clean();
} catch (\Throwable $e) {
    while (ob_get_level() > $_obInitialLevel_) {
        if (!@ob_end_clean()) {
            ob_clean();
        }
    }
    throw $e;
}

Output buffering is used by the framework to render views widgets among others. Currently PHP file rendering properly checks for output buffer states in error handling, but not in happy flows.

3. When rendering a PHP file, the rendering could (and should?) always check that the number of nested buffers is the same before and after rendering. If it differs we should either:

• Throw an error (Clean up your buffers when you're done, Yii is not your mom!)
• Flush the nested buffers until the nesting level returns to the expected value.

This issue was moved by samdark from yiisoft/yii2#16500.

@samdark commented on Jul 9, 2018, 10:36 AM UTC:

Sounds great. Would you like to help with it?

@samdark commented on Jul 9, 2018, 10:36 AM UTC:

Would it change current behavior of View for end user?

@SamMousa commented on Jul 9, 2018, 10:39 AM UTC:

Sounds great. Would you like to help with it?

Of course, if I make a proposal you may assume I'm willing to implement / help with it :-D

Would it change current behavior of View for end user?

Number 2 would, since the view is accessible via $view instead of $this (or whatever name we choose).
Note that these are 3 small proposals so we can do 1 and 3 but not 2 if you prefer.

Number 3 could create errors for users that do not properly use output buffering. But in those cases they are now probably rendering half their view content outside the layout... So we're helping them not hurting them by showing the error of their ways.

@samdark commented on Jul 9, 2018, 10:41 AM UTC:

1 and 3 are fine. 2 sounds very logical and, I think, it's possible to still use $this via bindTo or something like that.

@SamMousa commented on Jul 9, 2018, 10:43 AM UTC:

It definitely is, but then might as well skip it. The whole reason for using the approach is to not have a $this inside the view file.
If we want to have $this the current approach is just fine.

@samdark commented on Jul 9, 2018, 10:55 AM UTC:

Why not having $this is a good idea? How would you access $title, $metaTags, beginBody(), registerCsrfMetaTags() etc.?

@SamMousa commented on Jul 9, 2018, 11:02 AM UTC:

Via $view. There are several arguments for this, but none are very strong.

1. Static analysis / IDEs are not happy when they see $this used outside an object.
2. The name of the variable doesn't tell us what it means whereas $view does.

I realize these are not not particularly strong arguments for changing a known convention, but I thought I'd throw it out there.
$this is a reserved keyword and the way we're including it causes it to be available. Technically one could argue that this violates PSR-1. PSR-1 requires that each class must be in a file by itself.
But our views are included in our class and they use $this, so the View class is not in a file by itself...

@samdark commented on Jul 9, 2018, 11:57 AM UTC:

That makes sense actually, but:

1. $this is a convention we have for a long time and it works.
2. $view is very general as a variable in a template and could stand for database view, boolean flag, action etc. Could be confusing if we'll reserve that name.

@SamMousa commented on Jul 9, 2018, 12:30 PM UTC:

Fair enough, so we do not do 2, just and 1 and 3?

@samdark commented on Jul 9, 2018, 6:53 PM UTC:

Yes.

[samdark]

yiisoft/view@f3ec80a