Problem with PHP type in setter value type with Prado tpl/Page.

[zendre4]

If we use Php type in parameter of a setter function, the expected result is not correct. Example, with a simple component :

<?php

use Prado\Web\UI\ActiveControls\TActivePanel;

class TTest extends TActivePanel {
    public function setValue(bool $value): void {
        $this->setViewState("Value", $value, true);
    }
}

When we set the value property on a tpl like this :

<com:TTest Value="false" />

The value is true instead of false

[ctrlaltca]

All the code inside prado uses this method to ensure proper conversion of the string to a boolean value:

TPropertyValue::ensureBoolean($value);

Php itself doesn't care about the contents of the string ("false" or "true") when casting to bool: https://www.php.net/manual/en/types.comparisons.php

[zendre4]

@ctrlaltca

Me and my colleagues know that you should not type this setter parameters but use the phpdoc parameters instead (ex: string|bool),

But as we type more and more our php functions, it happens more and more that we make mistakes, so if we found a solution for don't do an exception for this setter function, it would be much better

[belisoful]

🖖My hot take: Nothing in PRADO would use this [yet], only third party developers. It is very inefficient in re-creating the reflection for each property rather than caching the class/property/type in a static variable. EVERY property of every object in a page will be reflected for this each time, always. Some pages are so large they take 1+ seconds to compute their template. Adding this much processing for something not used by PRADO but only in the remote chance of a developer using this isn't proper for merge.

Also, what about strings? Can you explain why strings are not supported?

It is an interesting idea where only simple known (and non-null) inputs are required. much of my PRADO work revolves around setter accepting null, but maybe not for you.

This could make PRADO a bit more automatic for developers; and i like that aspect. PRADO would need to be regressed for this update, so as to make any simple setters (throughout PRADO) into typed parameters. This isn't complex but is work inclusive of this change. This is not a "simple" addition as initially thought. I'd like to see that regression included in the change.

The new TDot could use this in regards to the "shadow transparency" property of the dot which is always a float value. The setter is as simple as can be. It uses ensureFloat, of course. but could just be a typed setter method.

I foresee one issue with this regarding behaviors. Behaviors can add their own properties and setters to a TComponent. So before merging, it would need to check all the behaviors for their properties and type. aaaaand that requires #826 to be fixed. The method_exists would need to use the new TComponent::hasMethod() to be behavior aware from #825.

Behaviors adding properties to WebControls is a use case for behaviors.

I am actively working on #825 and #826 and will be merging shortly.

[fabioman]

The problem right is that we can't type functions correctly.
If we have a class like this

<?php

use Prado\Web\UI\ActiveControls\TActivePanel;

class TActivePanelExt extends TActivePanel {

    public function setIsReadOnly(bool $value): void {
        $this->setViewState("IsReadOnly", $value);
    }
}

And in our page template we have

<com:TActivePanelExt IsReadOnly="false" />

The BIG PROBLEM right here is that even if we set that value in our page template as false, "IsReadOnly" value will always be set as true

[belisoful]

It's an issue, yes. The merge request code provided is incomplete and could be sped up greatly. PRADO would need a scrub to type all simple setters. That would be ideal.

[zendre4]

I made some modifications in the code of my pull request especially for the string

[belisoful]

Here is your hasMethod belisoful@dcf24bf
the typo fix: belisoful@abaaba3

The getBehaviors is coming shortly. The unit Tests need to be written.

[belisoful]

#826 getBehaviors is committed belisoful@e2e18d2

to be merged soon. There are a few more bugs I'd like to squash before submitting the merge.

[belisoful]

Ideally, each Reflection would only be done once, and the type retrieved only once as well.

[belisoful]

wou/d this issue include a scrub of master to implement it across the platform?