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

make the ManagerRegistry service configurable #165

Merged
merged 1 commit into from
Aug 31, 2014
Merged

Conversation

lsmith77
Copy link
Member

fix for #73

@stof
Copy link
Member

stof commented Aug 25, 2014

-1 for exposing it in the semantic config. It is a complex edge case, and the custom class might require more changes than just the class name (more dependencies to determine the manager for instance).

IMO, people wanting this should either overwrite the parameter themselves directly, or use a compiler pass to edit the definition if they need more stuff (or use the new decorates stuff in Symfony 2.5+). Cluttering the smeantic config with this is not worth it IMO

@dantleech
Copy link
Contributor

What if instead of the specifying the class name we specify the service ID?

We can then use an alias to define doctrine_phpcr and assign it to the configured service ID.

@lsmith77
Copy link
Member Author

in some places we allow configuring class names and in others services. both works for me i guess. but what else would you want to inject given that the registry has access to the container?

@dantleech
Copy link
Contributor

don't know - but you have the option of not injecting the container I guess.

@lsmith77
Copy link
Member Author

@stof well we were saying this is the solution for be able to do stuff like switching elegantly between a staging and production workspace. as such it is not such an edge case for quite a few CMS. but ok lets make the service configurable rather than the class.

@lsmith77 lsmith77 force-pushed the manager_registry_class branch from 3b42cb3 to c4da9de Compare August 29, 2014 10:09
@lsmith77
Copy link
Member Author

done

@lsmith77 lsmith77 added this to the 1.2 milestone Aug 29, 2014
'dump_max_line_length',
'workspace_dir' => 'workspace_dir',
'jackrabbit_jar' => 'jackrabbit_jar',
'dump_max_line_length' => 'dump_max_line_length',
Copy link
Member

Choose a reason for hiding this comment

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

why do you expand these? its not adding anything. i can live with it, just don't see why.

Copy link
Member Author

Choose a reason for hiding this comment

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

right. this was from a previous version of the patch. will remove

@dbu
Copy link
Member

dbu commented Aug 30, 2014

+1 on the way its done now, allowing to specify the service id for full customization. user can still use the di container parameter if he just needs a different class.

@lsmith77 lsmith77 force-pushed the manager_registry_class branch from c4da9de to c003b58 Compare August 31, 2014 17:27
@lsmith77 lsmith77 changed the title make the ManagerRegistry class configurable make the ManagerRegistry service configurable Aug 31, 2014
dbu added a commit that referenced this pull request Aug 31, 2014
make the ManagerRegistry service configurable
@dbu dbu merged commit b4ac2ea into master Aug 31, 2014
@dbu dbu deleted the manager_registry_class branch August 31, 2014 17:48
@dbu
Copy link
Member

dbu commented Aug 31, 2014

thanks. created symfony-cmf/symfony-cmf-docs#558 so we don't forget about the doc.

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

Successfully merging this pull request may close these issues.

4 participants