Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FEATURE: Allow usage of is*/has* accessors in Fluid templates directly #108

Merged
merged 3 commits into from
Apr 16, 2016

Conversation

albe
Copy link
Member

@albe albe commented Oct 27, 2015

This changeset adds support for accessor methods is* and has* to be
used directly for property access.
This allows to use such accessors in Fluid templates, which makes
the template code more readable and avoids getIs* and getHas* methods
in domain models.

Example::

<f:if condition="{someObject.isSomething}"></f:if>

This will call someObject->isSomething() method.

@aertmann
Copy link
Collaborator

Not sure why this is necessary, doesn't https://github.com/neos/flow/blob/master/Classes/TYPO3/Flow/Reflection/ObjectAccess.php#L166 already do this? AFAIR object.hidden calls isHidden if available.

@albe
Copy link
Member Author

albe commented Oct 27, 2015

Yes, but the use case is to use the more explicit and expressive "object.isHidden" syntax inside Fluid. This would currently create the necessary getter "getIsHidden()" on the object, which is suboptimal for code readability.

Edit: Note to self: Change IDE indenting to whitespace...

@albe albe force-pushed the ishas_access branch 3 times, most recently from 35eaddb to 2d54367 Compare October 27, 2015 17:51
@dfeyer
Copy link
Contributor

dfeyer commented Oct 29, 2015

👍

Maybe we need to check with @bwaidelich if that be in sync with the standalone fluid ?

try {
$subject = ObjectAccess::getProperty($subject, $pathSegment);
if (preg_match('/^(is|has)([A-Z].*)/', $pathSegment, $matches) > 0) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more a theoretical issue probably, but this won't match all allowed php method names starting with "is" or "has". Checking with substring like it's done in ObjectAccess would be safer probably

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind - as you're only comparing the first letter after "is"/"has" this is not a real problem!

@bwaidelich
Copy link
Member

Nice one, thanks for this.
Apart from the nit picks I still think this is a breaking change currently if you did go the extra round and implemented a method getIsFoo() which won't be called any longer with this change. So maybe we should try first whether the unmodified property name is gettable:

if (ObjectAccess::isPropertyGettable($subject, $propertyName)) {

and only change it if that's not the case

@albe
Copy link
Member Author

albe commented Jan 21, 2016

Thanks for the feedback, will push new commits soonish.

Edit:
Regarding the isPropertyGettable: this will not work if the object in question has a magic _call method, in which case it will always try to go the old way of calling "getIs/Has", which might fail even though the _call method generates magic "is" methods. Hence the double try/catch is unavoidable.
However, there is another big issue: the ObjectAccess::getProperty() will always prefer get* method over is* and checks that with is_callable. Hence in the case of magic __call method, it will always try to call "getSomething" then, instead of "isSomething", which is unexpected and would break. Hence we need to explicitly try to call $subject->$propertyName() instead.

@albe
Copy link
Member Author

albe commented Jan 21, 2016

@bwaidelich I think it's more readable now and fully b/c. I also split the tests and added another one for the getIs/Has* case.

PS: The failing styleCI check is unrelated, seems some CI conflict got merged/styleCI config is wrong?

@albe albe changed the title [FEATURE] Allow usage of is*/has* accessors in Fluid templates directly FEATURE: Allow usage of is*/has* accessors in Fluid templates directly Jan 21, 2016
@albe
Copy link
Member Author

albe commented Jan 21, 2016

Fixed the StyleCI issue with a rebase

@bwaidelich
Copy link
Member

👍 by reading. Thanks for taking care!!

@kitsunet
Copy link
Member

👍 looks good

@albe
Copy link
Member Author

albe commented Apr 2, 2016

Just added a small documentation reference to the new behaviour. Will merge with previous votes.

@albe
Copy link
Member Author

albe commented Apr 2, 2016

Rebased, maybe this helps the Travis hickup...

This changeset adds support for accessor methods is* and has* to be
used directly for property access.
This allows to use such accessors in Fluid templates, which makes
the template code more readable and avoids getIs* and getHas* methods
in domain models.

Example::
    <f:if condition="{someObject.isSomething}"></f:if>

    This will call someObject->isSomething() method.
- Fix is/has* case ending up calling get* instead
@albe
Copy link
Member Author

albe commented Apr 16, 2016

Ok, after the Travis issue is fixed, will merge with previous votes now.

@albe albe merged commit 2a4a963 into neos:master Apr 16, 2016
@albe albe deleted the ishas_access branch April 16, 2016 11:21
kdambekalns pushed a commit to kdambekalns/flow-development-collection that referenced this pull request May 8, 2016
… paths

This fixes the try/catch for undefined Fluid variables which start with "is" or "has".
Currently, exceptions here are not being caught because an `\Error` is being thrown.
As a safety measure for PHP7, we're now catching any `\Throwable` as well.
Also added is_callable check.

Specific example: a fluid section/partial with an optional argument called "isSomething"

- if rendered while defining isSomething in the arguments, there's no problem
- if rendered without isSomething in the arguments (implicit falsy), we then get
  `Call to undefined method TYPO3\Fluid\Core\ViewHelper\TemplateVariableContainer::isSomething()`

Related to: neos#108, neos#343
kdambekalns added a commit that referenced this pull request May 9, 2016
BUGFIX: Fix ObjectAccessorNode::getPropertyPath for is/has access in PHP7

This fixes the try/catch for undefined Fluid variables which start with "is" or "has".
Currently, exceptions here are not being caught because an `\Error` is being thrown.
Since non-callable method call should not be attempted in the first place, this change
adds a is_callable check. Therefore a try/catch block is no longer necessary, as any exception thrown inside the callable method should bubble up.

Specific example: a fluid section/partial with an optional argument called "isSomething"

- if rendered while defining isSomething in the arguments, there's no problem
- if rendered without isSomething in the arguments (implicit falsy), we then get
  `Call to undefined method TYPO3\Fluid\Core\ViewHelper\TemplateVariableContainer::isSomething()`

Related to: #108, #343, #348, whythecode/flow-development-collection#1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants