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.