Open smnandre opened 2 weeks ago
Twig already throws an error if you attempt to access a variable that doesn't exist, so isn't the exception in ComponentRenderer redundant?
I wonder if it would make sense to both "not expose uninitialised properties" and "not throw an exception in ComponentRenderer".
That way, the documented contract would be consistent with the behaviour, and you'd allow the situation to be handled in the template itself ({% if uninitialisedExposedProperty is defined %}
).
This is exactly the situation i want to avoid, as it does not respect the contract.
Again, as i see it .. if you want to use unitialized properties, you can, via this.fooBar
But you need to disable exposePublicProps
Precisely what is the contract? "Every public property must be given to Twig."?
Either way, I agree with parts of both ends of the argument and I'm fairly agnostic to what the outcome will be. Adhering to a clear contract, when there is one, is IMO better.
My real concern on that day revolved about the fact something that worked for a long time abruptly stopped working between two minor versions, with neither a grace period where a deprecation would appear instead of a flat-out exception, nor upgrade notes with a warning.
TL;DR; i regret the revert and think we should throw an expection
So...
I think i now regret to have reverted #1780 (after #1909 but also messages on Slack)
On that moment, i just wanted to limit / avoid pressure, stress, and conflict.
But now... i'm more than ever convinced we should (must?) throw an exception when a component has an unitialized public property and
exposePublicProps = true
Why is this not a BC break
It has never been said one could use unitialized property to not expose one inside the template.
Source code
In the source code of the AsTwigComponent attribute
Please note the every here.
Documentation
In the online documentation, you can read the following
No mention to "initialized". And if you look, the example is matching our discussed scenario
Then, it explicitely gives a method if you don't want to expose them, and access them via
this.
object (your component)Why we should throw an Error
Well, we cannot expose a "undefined" in Twig. Every public property must be given to Twig.
Responsability
If a component exposes a property, one should be able to call them without having to fear any "undefined".
And if the message is typed "string", there is no reason to test this in the template.
The responsability to pass this value must be set on the person calling the component.
And if you need a string|null value... well, you also can make the property nullable.
DX / Debug
Same, in the debug command, we would not say "this prop is unitialized" but only its type and eventually default value.
Now what ?
So i really think we made the good choice initially.
We now need to decide what we want to do now... two options: 1- keeping the current status (that caused and still causes issues), while not enforcing the contract we documented 2- explaining people they used an undocumented "gray area", and showing them solution to change/adapt (as the change is possible on this direction, while the other one maintain a problematic situation)
--
I do think the 2 is the way to go, and i'm open to any feedback on this :)