Fix/bug with multiple view renders

[fadrian06]

NOTE: IT' S NOT A BREAKING CHANGE!!!

See problem #577

Component

<div><?= $prop ?></div>

Flight::render('myComponent', ['prop' => 'Hi']);
Flight::render('myComponent');

Before: this is by default

<div>Hi</div>
<div>Hi</div>

After: activable through a flag

<div>Hi</div>
<div>PHP Warning: Undefined variable $prop</div>

NOTE: One way of does not have a warning is to provide a default value within your components

// my component
if (!isset($prop)) {
  $prop = 'default value';
}

// or since PHP 7.4
$prop ??= 'default value';

To enable just:

Flight::view()->preserveVars = false;

// or if you're using View instance directly
$view->preserveVars = false;

[fadrian06]

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Runtime: PHP 8.2.18
Configuration: C:\xampp\htdocs\libs\flight\phpunit.xml
Random Seed: 1714003323

............................................................... 63 / 290 ( 21%)
............................................................... 126 / 290 ( 43%)
............................................................... 189 / 290 ( 65%)
............................................................... 252 / 290 ( 86%)
...................................... 290 / 290 (100%)

Time: 00:03.610, Memory: 10.00 MB

OK (290 tests, 485 assertions)

---

PHPUnit 9.6.19 by Sebastian Bergmann and contributors.

Runtime: PHP 7.4.33
Configuration: C:\xampp\htdocs\libs\flight\phpunit.xml
Random Seed: 1714003397

............................................................... 63 / 290 ( 21%)
............................................................... 126 / 290 ( 43%)
............................................................... 189 / 290 ( 65%)
............................................................... 252 / 290 ( 86%)
...................................... 290 / 290 (100%)

Time: 00:03.168, Memory: 10.00 MB

OK (290 tests, 485 assertions)

[fadrian06]

Don't merge yet

There should still be an exception to it not being executed regardless of the flag and that is if you provide a key to render

[sandebert]

Surely #577 must be considered a straight-up bug? It does seem strange to me to preserve the buggy behaviour with a flag.

[fadrian06]

to preserve the buggy behaviour with a flag.

What happens is that Flight tries as much as possible not to produce backward compatible changes and even less so in a bug fix, we are preparing the changes that will be breaking for version 4

[fadrian06]

The current behavior, although technically it is a bug, you can take advantage of it, such as rendering a piece of UI with certain variables, and save yourself from having to repeat them in consecutive renders of the same component

[fadrian06]

So we assume that at least half of Flight::render users do it like this, and that's why we don't want to break that code, but you are given the option to disable that behavior of preserve variables between renders

[vlakoff]

The current draft would remove existing items from $this->vars, when the $data parameter contains items having the same name. But I think the correct behavior should be to preserve these items. The point of the new "not preserveVars" mode is to avoid adding the $data parameter's items to the $this->vars property.

I would suggest this code instead:

\extract($this->vars);

if (\is_array($data)) {
    \extract($data);
    if ($this->preserveVars) {
        $this->vars = \array_merge($this->vars, $data);
    }
}

(we might add the EXTR_OVERWRITE flag – although it is already the default – in the second extract(), for more expliciteness)

[vlakoff]

Also, a test might be added for this. Something like:

$view->preserveVars = false;
$view->set('foo', 'bar');
$view->render('template', ['foo' => 'qux']);
$view->render('template'); // expected: $foo still exists, and equals 'bar'

[n0nag0n]

@vlakoff I don't hate the code you suggested. I would make sure it fits in the unit tests (and add your unit test you suggested as well).

The only thing I don't like about it is that I hate extract() in general haha. Debugging extract becomes really hard though in this case I think it's fine. A user trying to debug why their variable is missing might be confused at which extra is causing the issue.

One other thought I had..... what if we added to this code similar to https://github.com/flightphp/core/blob/master/flight/net/Router.php#L255-L264 If you can't find your variable name, scan through the array keys of $this->vars/data supplied and give a suggestion for what you might have meant instead? Latte does this, and does a really awesome job at it. It's really helpful with debugging!

[n0nag0n]

Just checking in on this. What do you think @fadrian06 ?

[fadrian06]

I've been testing the development branch of this PR and it's apparently working as expected

[fadrian06]

Obviously when I activated the flag my views broke due to the bad behavior of preserving state between renders

[fadrian06]

You can see the project I'm using it in Aime309/sarco at v3
https://github.com/Aime309/sarco/tree/v3

here I am activating the flag sarco/app/configurations/views.php at v3 · Aime309/sarco
https://github.com/Aime309/sarco/blob/v3/app/configuraciones/vistas.php

here an implementation of an input component sarco/views/components/Input.php at v3 · Aime309/sarco
https://github.com/Aime309/sarco/blob/v3/vistas/componentes/Input.php

Note that there are component attributes that are required and others optional, but with the flag it is always reset to that state by default

here a login implementation
the required props are passed and always are passed because the previous state is deleted, the optional props sometimes are passed, sometimes aren't

sarco/vistas/paginas/ingreso.php at v3 · Aime309/sarco
https://github.com/Aime309/sarco/blob/v3/vistas/paginas/ingreso.php

[fadrian06]

The current draft would remove existing items from $this->vars, when the $data parameter contains items having the same name. But I think the correct behavior should be to preserve these items. The point of the new "not preserveVars" mode is to avoid adding the $data parameter's items to the $this->vars property.

Thank you for your contribution, seeing it in detail accompanied by the test, if it makes sense to keep the vars that are assigned with ->set(...), I have to review it because there are many more cases to test

[fadrian06]

@n0nag0n a var_dump before extract and that's it, it's not like extract will get variables out of nowhere, it will always take the keys from the array... for me the fewer lines the better, fewer bytes and more flight..

Also, there was a tip that is usually given in Python that I liked.
If the language has a native function for what you are trying to do, which is to use variable variables, which is even more confusing... better use the native function which is most likely to be more optimized.

[vlakoff]

I noticed a possible source of confusion. The undesired behavior may be encountered on the next render() call actually, as the unset() are called after the rendering of the view.

Proper version of my snippet from #581 (comment):

Current behavior:

$view->preserveVars = false;
$view->set('foo', 'bar');
$view->render('template', ['foo' => 'qux']);
$view->render('template'); // in the view rendered here, $foo equals 'qux' (set from previous call)
$view->render('template'); // in the view rendered here, $foo doesn't exist (deleted from previous call)

Expected behavior:

$view->preserveVars = false;
$view->set('foo', 'bar');
$view->render('template', ['foo' => 'qux']);
$view->render('template'); // in the view rendered here, $foo still exists, and equals 'bar', from the ->set()

Using the $data parameter should not change $this->var: not changing existing values, nor deleting existing values. To say it otherwise, the $data parameter should be an override for the current view only. Whereas using ->set() defines variables for each subsequent renderings, unless "locally" overriden by $data for a given call.

[vlakoff]

I would say the preserveVars name is not optimal for the property, because we are not preserving the vars property, but rather persisting the overrides. Suggestions: preserveOverrides, keepOverrides, persistOverrides…

Also, a short docblock should be added to the property. Because what this property does should really be documented in the code, and also for consistency with the other properties, which have a docblock.

[vlakoff]

However, specifying a $data argument in a render call isn't necessarily an "override"…

The name of the property is only a secondary consideration. To me, the current "persisting" behavior is clearly wrong and we should get rid of it, the sooner the better.

Users should right now update their codes to replace $data arguments with $view->set(...) calls, when they want to persist variables.

[fadrian06]

If you are completely right that now that the target of the fix changed the variable should also change

[n0nag0n]

I'm good with how this is now. I see your point @vlakoff with the naming of it, and I'm one of the first to stop and argue about what something is named for hours (I do it at work all day haha!) I think the naming of this is alright though. The View class I almost feel should be deprecated in favor of a more dynamic and powerful templating platform like Smarty/Twig/Latte, but since this is so basic, it makes sense. @fadrian06 Don't forget to update the docs!

[vlakoff]

(off topic)

The View class I almost feel should be deprecated in favor of a more dynamic and powerful templating platform like Smarty/Twig/Latte

I disagree with this, these template engines add a whole layer of complexity (new syntax to learn, caching mechanism to implement because compiling is so slow), while bringing little-to-no benefits compared to bare PHP views.

Though, I'm fine with introducing template engines, as long as they remain fully optional.

For instance, Symfony is advertised as being soooo modular, but when overriding the managing of HTTP 404 responses, we are tied to use a Twig view…

[n0nag0n]

I've tried and built projects with Twig and I really disliked Twig. I've seen Smarty's syntax and I don't like it much either. Latte on the other hand, has been very valuable for me. The more I learn about Symfony the less I like their stuff even though people swear by it. For serving simple pages, Flights stuff is just fine. For anything more than complex, I'd definitely recommend a templating engine like Latte.