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

ContainerBuilder: removed support for ? (BC break) #93

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

dg
Copy link
Member

@dg dg commented Jan 27, 2016

We should Identify all cases when ? is used, resolve them and remove a question mark (it is like eval).

@enumag
Copy link
Contributor

enumag commented Jan 27, 2016

So hypothetically... what should I use instead of

->addSetup('$service->methodA(?)->methodB(?)', ['a', 'b']);

@dg
Copy link
Member Author

dg commented Jan 27, 2016

->addSetup([new Statement('@self::methodA', ['a']), 'methodB'], ['b']);

@enumag
Copy link
Contributor

enumag commented Jan 27, 2016

Yep, that's what I was afraid of. 👎

@dg
Copy link
Member Author

dg commented Jan 27, 2016

Why 👎? Is it forbidden to invent some syntactic sugar? Like chain('@self::methodA', ['a'], 'methodB', ['b'])?

@enumag
Copy link
Contributor

enumag commented Jan 27, 2016

First its a BC break that would force me to update quite a few of my extensions. Second the syntax using question marks is probably the best syntactic sugar you could come up with - the chain syntax from your last comment doesn't solve cases like @a->b(@c->d(?)). Also this would effect neon definitions as well right?

By the way you didn't mention why you want to remove the question marks in the first place. Right now I fail to see any benefit, only BC break and uglier syntax. Thats why 👎 for now but you might be able to convince me.

@dg
Copy link
Member Author

dg commented Jan 27, 2016

Because eval is evil. DI compiler is unable to understand what it means. It relies on internal implementation details, like name $service etc…

@enumag
Copy link
Contributor

enumag commented Jan 27, 2016

$service can be replaced with @self right?

@fprochazka
Copy link
Contributor

@dg I'm strongly against removing the ability to push custom code to the service creation. It's very handy and allows for hacking the container :)

I've went through most of my code, and the main reason for using the ? is appending to an array. So adding a syntax like this would be neccesary

$def->addSetup('$array[]', ["value"])
# converts to
$service->array[] = "value";

Imho it makes sense first to introduce new syntaxes to solve the causes for using the ? and remove/depricate it much longer after it's adopted.

@dg
Copy link
Member Author

dg commented Jan 27, 2016

@fprochazka see #93 (comment)

@fprochazka
Copy link
Contributor

@dg "eval is evil" is a statement taugh to begginers because they can create a lot of damage by using it wrong and also create exploits. It's not evil, just dangerous.

@fprochazka
Copy link
Contributor

@dg I never figured the array in setup's statement, that's a nice one.

@dg
Copy link
Member Author

dg commented Jan 27, 2016

new syntaxes to solve

I think it can be solved with current syntax (::array_push(@self::$array, value) in neon), but I am opended to enhance syntax.

@fprochazka
Copy link
Contributor

@dg I get your point, but I hope I'm not the only one who considers this very ugly :)

@fprochazka
Copy link
Contributor

My use-cases for custom code in service creation

There is more, but this represents the most common ones for me.

@fprochazka
Copy link
Contributor

@dg please reconsider the BC Break effect of deprecating this on extensions. I'm 👍 on adding the new syntax for array append, but 👎 on removing the ?.

@JanTvrdik
Copy link
Contributor

It may be interesting to use https://wiki.php.net/rfc/use_function#domain_specific_languages combined with https://wiki.php.net/rfc/group_use_declarations#rfc_impact instead of parsing magic strings.

@fprochazka
Copy link
Contributor

@JanTvrdik nice idea

@dg dg force-pushed the master branch 2 times, most recently from ba7b241 to fb96e4f Compare February 8, 2016 13:24
@dg dg force-pushed the master branch 2 times, most recently from 231a29c to 7f12a9f Compare April 21, 2016 13:03
@dg dg force-pushed the master branch 9 times, most recently from 9fbd1d4 to a71f267 Compare May 11, 2016 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants