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

Twig 2.9 assumes that the filter tag creates a new context scope #3091

Closed
ericmorand opened this issue Jul 22, 2019 · 12 comments
Closed

Twig 2.9 assumes that the filter tag creates a new context scope #3091

ericmorand opened this issue Jul 22, 2019 · 12 comments

Comments

@ericmorand
Copy link
Contributor

ericmorand commented Jul 22, 2019

This is what the documentation says now:

As of Twig 2.9, you should use the apply tag instead which does the same thing except that the wrapped template data is not scoped.

This was not the case before Twig 2.9: neither the documentation nor the test suite did say anything about the fact that the filter tag creates a new context scope. The documentation and the test were added on April 2019 (this commit: 619bc12).

Prior to that, the filter tag was not supposed to create a new scope. But it did on TwigPHP prior to 2.9. Which means that TwigPHP filter tag implementation was faulty.

Then, with TwigPHP 2.9, instead of fixing the bug, you have updated the documentation and added a test! Which is a breaking change.

Let me summarize:

  • The specifications don't say anything about the fact that the filter tag creates a new context scope
  • TwigPHP filter tag implementation creates a new context scope, which is a bug
  • Suddenly, you decide to change the specifications to match the faulty implementation of TwigPHP

What is the rational behind that behavior? Is it assumed at Twig, SensioLabs or whatever entity is behing Twig specifications that when TwigPHP is faulty, then it is not faulty? How are other Twig implementations supposed to guess what is a faulty behavior if the documentation and the test suite can't be used as reference?

Bonus question: when will you release the language specifications publicly?

@ericmorand
Copy link
Contributor Author

Let me elaborate on that and why this is an unacceptable and very aggressive move from you in the context of open source.

Every other implementations is now stuck with a breaking change. You never said explictly nor implicitly that the filter tag was creating a context scope prior to April 2019. So every other implentation assumed - rightfully, that the filter tag did not create a new scope.

Now that you discover that TwigPHP is faulty, instead of fixing the bug, you declare the bug as part of the specifications. And you get away with it, letting all other implementations having to provide a major bump to fix that bug because it creates a breaking change.

Because you didn't want to bump one single project (yours) to a new major version, you force an undefined number of other projects to bump to a major version.

This is an extremely aggressive move. And an explicit "we don't care about you" message to the open source community.

@stof
Copy link
Member

stof commented Jul 22, 2019

The filter tag has always created a new scope (not intentionally). This is exactly why a new apply tatg was created and filter deprecated: to move away from this behavior.

@stof
Copy link
Member

stof commented Jul 22, 2019

Also, we don't have a specification for Twig different from the PHP Twig implementation. There is nothing like a twig spec. There is the documentation of the Twig engine.

@ericmorand
Copy link
Contributor Author

The filter tag has always created a new scope (not intentionally).

It was not in the doc. Not in the tests. And you agree that it created a scope not intentionally. It's the definition of a bug.

There is the documentation of the Twig engine.

Well, if you talk about the available documentation, then once again it says nothing about the fact that the filter tag creates a scope.

So, let me ask again: why has it not been fixed in Twig 2.9 instead of updating the doc to match the "not intentionally" behavior of Twig < 2.9?

@stof
Copy link
Member

stof commented Jul 22, 2019

even if it was not intentional, changing the scoping behavior would affect existing project. Deprecating the existing tag in favor of a new one was much better for the community of Twig users than silently changing the behavior of this tag (which would be a breaking change for any project depending on the scoping behavior available since the first release of the tag).

@ericmorand
Copy link
Contributor Author

ericmorand commented Jul 22, 2019

I don't understand. Nobody complained about that behavior - that filter creates a scope - or an issue would have been created (maybe I missed it).

What was the urgency to fix something that nobody was complaining about ? Why not wait for Twig V3 which allow breaking changes.

Even it is was urgent, that's what major versions are made for. Why not issuing Twig V3 with just that change if this is something urgent? You could have included that other breaking change from Twig 2.11.

I really don't get your point. You had many safe options that comply with semver and you chose the one that doesn't.

@stof
Copy link
Member

stof commented Jul 22, 2019

Well, it became an issue when we started recommending {% filter spaceless %} instead of {% spaceless %} as a way to apply spaceless on whole block, as that would introduce a scope.

And as I said, the breaking change would precisely be to remove the scoping behavior. Existing projects care about the implementation, not about what the doc says or not, regarding what breaks them. Semver agrees with us: it cares about changes done in the package you distribute, not about changes in its doc.

@ericmorand
Copy link
Contributor Author

ericmorand commented Jul 23, 2019

Semver agrees with us: it cares about changes done in the package you distribute, not about changes in its doc.

This is highly debatable. You are right in the sense that semver clearly states that the public API is allowed to be "enforced by the code itself", which is the case there. But it also clearly states that "it is important that this API be clear and precise", which is not the case there.

You would be totally right if the code was documented explictly by saying that the filter tag was creating a new scope. It would have matched both the "enforced by the code itself" and the "clear and precise" parts.

I keep on thinking that the scope issue was a bug (it was not intentional and not written either in the public documentation or the code documentation) and that you enforced a documentation change to match a bug in your implementation.

How can we be confident that you won't update the documentation/specifications each time you discover a bug in your implementation? You are judge and party here. The correct solution would have been to update your documentation saying "There is a bug in TwigPHP that makes the filter tag create a new context scope. It's a known issue but it's unfortunately too late to fix it. It will be fixed in TwigPHP v3." This is totally acceptable and quite common in language implementations - look at the list of "known issues" of ECMAScript implementations.

But what you made was not admitting that TwigPHP was faulty and pretend that it was planned all along - at least that's what the documentation implies, I had to ask here to know that it was not intentional and thus a bug.

I'll adopt this strategy myself. Add the fact that filter tag doesn't create a new context scope to the list of known issues of Twing. To not have to bump a new major version for such a tiny issue - and a tag that is deprecated.

@stof
Copy link
Member

stof commented Jul 23, 2019

@ericmorand you are comparing that to ecmascript implementations. But that's wrong. Ecmascript is a language specifications developed as such and having multiple ("official") implementations. This is not what Twig is. Twig is a PHP templating engine. Any port to other language is totally unofficial, and there is no specification independent of the PHP implementation.

Also, the Twig documentation is not written as a language specification. A specification would have to be a lot more precise about behavior for all corner cases, and would then become useless as a documentation (are you actually reading the Ecmascript spec to learn using JS ? I don't think so).

@ericmorand
Copy link
Contributor Author

(are you actually reading the Ecmascript spec to learn using JS ? I don't think so).

Actually I do. I also read some part of PHP specifications and its source code in C to create Twing - there are a lot of PHP implementation details that are not explicit in the docs and that Twig consider as a given - output buffering, all-over-the-place comparison rules, behavior of various array-related functions depending on the keys, and so on. So yes, I read and teach language specifications. Not saying that I know them by heart of course. But it's definitely something that ever serious developer should do instead of relying on tutorials or official documentations.

Anyway, is "there is no specification independent of the PHP implementation" planned to change in the foreseeable future?

@stof
Copy link
Member

stof commented Jul 23, 2019

Anyway, is "there is no specification independent of the PHP implementation" planned to change in the foreseeable future?

I don't know of any such plan.

@fabpot
Copy link
Contributor

fabpot commented Aug 7, 2019

  1. There is no such thing as Twig specification
  2. We did not break anything as filter has always created a new scope
  3. We introduced a new tag apply that we think behave in a better way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants