Closed fadrian06 closed 1 month ago
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)
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
Surely #577 must be considered a straight-up bug? It does seem strange to me to preserve the buggy behaviour with a flag.
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
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
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
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)
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'
@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!
Just checking in on this. What do you think @fadrian06 ?
I've been testing the development branch of this PR and it's apparently working as expected
Obviously when I activated the flag my views broke due to the bad behavior of preserving state between renders
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
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
@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.
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 https://github.com/flightphp/core/pull/581#issuecomment-2081295782:
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.
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.
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.
If you are completely right that now that the target of the fix changed the variable should also change
(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…
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.
See problem #577
Component
Before: this is by default
After: activable through a flag
To enable just: