search

thephpleague / tactician-bundle

Bundle to integrate Tactician with Symfony projects
MIT License
245 stars 43 forks source link

Enriches the README with Symfony 3.3 specific config #55

Closed pderaaij closed 7 years ago

pderaaij commented 7 years ago

I might be adding the obvious, but it costed me two hours to figure it out.

In order to make this bundle work registered services needs to be marked public. I've added an example to the README.

chalasr commented 7 years ago

That's not true anymore :) but that's not released yet. Not sure if this should be merged as there is no v1 yet.

pderaaij commented 7 years ago

Don't know if v1 is released soon, otherwise it would nice to add. I could add this will change with the release of v1. But it can help people who start using it right now.

rosstuck commented 7 years ago

Hmm, dang, that stinks and sorry it took so long to figure out.

Is it possible we do something else here or possibly set it always public in the compiler pass?

Robin, what are you referring to here exactly, an unreleased change in Symfony or the unreleased work in the bundle?

On Jul 17, 2017 2:07 PM, "Paul de Raaij" notifications@github.com wrote:

Don't know if v1 is released soon, otherwise it would nice to add. I could add this will change with the release of v1. But it can help people who start using it right now.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/thephpleague/tactician-bundle/pull/55#issuecomment-315737474, or mute the thread https://github.com/notifications/unsubscribe-auth/AAI9TsRurVtMZ2NuEMYvTrX6NnZA2z3Yks5sO05qgaJpZM4OZW0m .

chalasr commented 7 years ago

@rosstuck I mean that command handlers don't need to be public anymore, solved in #43 :) So not sure what should be done here, we don't have a branch for pre-1.0 releases in which this could have been merged.

rosstuck commented 7 years ago

Agreed. My proposal then is we leave this issue open to help folks who are dealing with this issue already and we turn up the heat on solving the security/testing issues that are blocking a 1.0 release.

We can merge this into the README but then it needs a statement saying this is a temporary solution because clearly this isn't what we want long term.

rosstuck commented 7 years ago

Alternately...we strip/disable the security features for the time being.

pderaaij commented 7 years ago

I've updated the PR to contain an additional note. I think it would be nice to merge this into the repo. Changes can be made when 1.0 will be released.

rosstuck commented 7 years ago

Thanks @pderaaij, sorry again for the hassle

pderaaij commented 7 years ago

@rosstuck Don't worry about it :) Thanks for merging!