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

fixed getNode in Expression_Array #2364

Closed
wants to merge 2 commits into from
Closed

fixed getNode in Expression_Array #2364

wants to merge 2 commits into from

Conversation

pedro151
Copy link

No description provided.

@stof
Copy link
Member

stof commented Jan 16, 2017

This is a BC break as it totally changes the behavior of these methods

@stof
Copy link
Member

stof commented Jan 16, 2017

Btw, you don't even describe what you want to fix

@pedro151
Copy link
Author

The way that data is retrieved from a given node in the class is best having an associative index

@stof
Copy link
Member

stof commented Jan 16, 2017

You are changing totally the behavior of these methods.

And this means that the behavior of these methods does not correspond to the Twig_Node contract anymore.
Btw, the testsuite shows that this breaks things.

getNode and hasNode is not meant to access properties of the array by key, but accessing Twig nodes in the AST.

Thus, the API you suggest is broken. Keys in pairs are not strings. They may be very complex Twig expressions. There is no way to get the value of an array for a given key using just the AST, as actual keys are not known until the execution of the template.

@stof
Copy link
Member

stof commented Jan 17, 2017

Please revert all permission changes. Files must not be executable

@@ -1,28 +1,10 @@
{
"name": "twig/twig",
"name": "pedro151/twig",
Copy link
Member

Choose a reason for hiding this comment

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

this is a no-go

}

public function compile(Twig_Compiler $compiler)
public function compile ( Twig_Compiler $compiler )
Copy link
Member

Choose a reason for hiding this comment

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

why are you changing coding standards ? If you want it to be merged, this must be reverted (and if you don't want it to be merged, you should close the PR)

@pedro151
Copy link
Author

Sorry, I sent files that should not be loaded and merged by themselves. Modifications already removed

@pedro151 pedro151 closed this Jan 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants