Make "length" filter and "empty" test consider __toString [Twig 1.x] #2420
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
LGTM. Can you add some tests and a note in the CHANGELOG file? |
|
@fabpot done. Hope the tests look like what you expected, otherwise please guide me. Failing tests on PHP 7 only, seems unrelated. |
This was referenced Mar 10, 2017
fabpot
reviewed
Mar 14, 2017
lib/Twig/Extension/Core.php
Outdated
| return mb_strlen($thing, $env->getCharset()); | ||
| } | ||
|
|
||
| if (is_object($thing) && !$thing instanceof \Countable && is_callable(array($thing, '__toString'))) { |
There was a problem hiding this comment.
I would use method_exists() here instead as __toString() must be public and we don't want to return true is a class implements __call().
There was a problem hiding this comment.
which means that another test with a class having __call() but no __toString() should be added as well.
mpdude
changed the title
mpdude
changed the title
mpdude
commented
Mar 15, 2017
doc/filters/length.rst
Outdated
| If ``Countable`` is implemented as well, the return value of the ``count()`` method | ||
| takes precedence. | ||
|
|
||
| .. versionadded:: 1.33 |
There was a problem hiding this comment.
@fabpot Do you have tooling/policy/processes in place to make this read 2.3 when merged into the 2.x branch?
There was a problem hiding this comment.
I will do that manually I think
There was a problem hiding this comment.
This block should be moved to the top.
|
@fabpot Do you think this could make it into the next Twig release? |
|
Can you rebase to get rid of the merge commit? I will then review it and make any additional comment before merge and release. Thanks. |
fabpot
approved these changes
Mar 22, 2017
CHANGELOG
Outdated
| * 1.33.0 (2017-XX-XX) | ||
|
|
||
| * "length" filter now returns string length when applied to an object that does not implement \Countable but provides __toString() | ||
| * "empty" test will now consider the return value of the __toString() method for objects implementing __toString() but not \Countable |
There was a problem hiding this comment.
Can you wrap each line like done for other items?
doc/filters/length.rst
Outdated
| If ``Countable`` is implemented as well, the return value of the ``count()`` method | ||
| takes precedence. | ||
|
|
||
| .. versionadded:: 1.33 |
There was a problem hiding this comment.
I will do that manually I think
doc/filters/length.rst
Outdated
| length of the string being returned. | ||
|
|
||
| If ``Countable`` is implemented as well, the return value of the ``count()`` method | ||
| takes precedence. |
There was a problem hiding this comment.
I find this confusing. I prefer the way you documented the other one. Can you change it to read the same as the other one?
doc/tests/empty.rst
Outdated
|
|
||
| .. versionadded:: 1.33 | ||
|
|
||
| Support for the ``__toString()`` magic method has been added in Twig 1.33. |
There was a problem hiding this comment.
This block should be moved to the top.
doc/filters/length.rst
Outdated
| If ``Countable`` is implemented as well, the return value of the ``count()`` method | ||
| takes precedence. | ||
|
|
||
| .. versionadded:: 1.33 |
There was a problem hiding this comment.
This block should be moved to the top.
…t are not \Countable
Use case: When you have variables in your views that are actually objects but implement `__toString`, they feel like strings: For example, `{{ something }}` will make use of that to-string-conversion.
What does *not* work is `{{ something | length }}`, because that will only have a meaningful return value for objects implementing `\Countable`. This interface, however, may have a totally different semantic/purpose for the object in question.
For template designers, this may be surprising if they don't actually care about the object-or-string difference, they just "use" the variable.
This change tries to address this as it changes the behavior for such objects that have a `__toString` method and are *not* `\Countable`.
*Yes*, it's a BC break in an edge case: Objects that implement a `__toString` but not `\Countable` would previously yield `1` for `{{ object | length }}`, and now would return the length of the string returned by `__toString`.
Both instanceof and method_exists() seem to handle non-object value just fine
mpdude
force-pushed
the
length-for-tostring-objects
branch
from
60c21a8
to
9dbed98
Compare
|
@fabpot rebased, docs fixed as you suggested |
|
Thank you @mpdude. |
fabpot
added a commit
that referenced
this pull request
Mar 22, 2017
…ng [Twig 1.x] (mpdude) This PR was squashed before being merged into the 1.x branch (closes #2420). Discussion ---------- Make "length" filter and "empty" test consider __toString [Twig 1.x] Use case: When you have variables in your views that are actually objects but implement `__toString`, they feel like strings: For example, `{{ something }}` will make use of that to-string-conversion. What does *not* work is **a)** `{{ something | length }}`, because that will only have a meaningful return value for objects implementing `\Countable`. This interface, however, may have a totally different semantic/purpose for the object in question. **b)** `{% if something is empty %}`, because > empty checks if a variable is an empty string, an empty array, an empty hash, exactly false, or exactly null `[http://twig.sensiolabs.org/doc/2.x/tests/empty.html]` ... and obviously `something !== null` in this case. For template designers, this may be surprising if they don't actually care about the object-or-string difference, they just "use" the variable. This change tries to address this as it changes the behavior for such objects that have a `__toString` method and are *not* `\Countable`. *Yes*, it's a BC break in edge cases: For a), objects that implement a `__toString` but not `\Countable` would previously yield `1` for `{{ object | length }}`, and now would return the length of the string returned by `__toString`. For b), testing (defined) variables that are objects implementing `__toString` and that return `''`, the test now is `false`. Commits ------- f5193e9 Make "length" filter and "empty" test consider __toString [Twig 1.x]
|
Not sure if it is BC or we are misusing toString method, but after updating to twig 2.3.0 our application crashed on this code. let's say you have an entity with a __toString method that not always returns a string. When you do something like: Is this related to this PR? @fabpot @mpdude . If we downgrade to 2.2.0 it works. Thanks for your time. |
|
The |
|
In PHP? |
|
@mpdude |
My question is, should I mean, if the object is not null, it is not empty, no need to check more things. That's why I asked, I wasn't sure if I am missing something there. |
|
What is your code supposed to do? If |
|
If |
|
I can make a PR to make the But consider I don't see how this could work "both" ways. |
Merged
|
We used |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use case: When you have variables in your views that are actually objects but implement
__toString, they feel like strings: For example,{{ something }}will make use of that to-string-conversion.What does not work is
a)
{{ something | length }}, because that will only have a meaningful return value for objects implementing\Countable. This interface, however, may have a totally different semantic/purpose for the object in question.b)
{% if something is empty %}, because... and obviously
something !== nullin this case.For template designers, this may be surprising if they don't actually care about the object-or-string difference, they just "use" the variable.
This change tries to address this as it changes the behavior for such objects that have a
__toStringmethod and are not\Countable.Yes, it's a BC break in edge cases:
For a), objects that implement a
__toStringbut not\Countablewould previously yield1for{{ object | length }}, and now would return the length of the string returned by__toString.For b), testing (defined) variables that are objects implementing
__toStringand that return'', the test now isfalse.