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

Add missing service properties #145

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

Conversation

jaypea
Copy link

@jaypea jaypea commented Dec 3, 2013

this adds missing properties from symfony services to the @service annotation.
this is basically a copy of #115 with the "lazy" field of a service and fixed unit tests. thanks to @molchanoviv for starting this.

my use case is slightly different so i want to bring this issue back to your attention.
I use the Annotations to configure my services in the class itself, instead of maintaining xml or yml config files. to be fully compatible with the other configurations, i need the factoryService and factoryMethod and since symfony now has the "lazy" option for services, i decided this would be helpful as well.

My use case is a genery cache proxy for method calls to services. the factory in my case does not know, which service it will provide a proxy for, its all configured in the service itself.

@@ -64,6 +64,19 @@ public function convert(ClassHierarchyMetadata $metadata)
$definition->setTags($classMetadata->tags);
$definition->setProperties($classMetadata->properties);

if (null !== $classMetadata->lazy) {
$definition->setLazy($classMetadata->lazy);
Copy link

Choose a reason for hiding this comment

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

Shouldn't there be a check for setLazy using method_exists (cf. initMethod below)?

Copy link
Author

Choose a reason for hiding this comment

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

good idea, will commit a change.

@schmittjoh
Copy link
Owner

Could you check the minor indentation issue that @stof mentioned? If you can have a look at why Travis fails that would cool too.

@molchanoviv
Copy link

Hey guys. I think we should write new Factory annotation like it was said here #115

@jaypea
Copy link
Author

jaypea commented Dec 16, 2013

@schmittjoh fixed the indentation issue.
travis fails for the same reason, the master branch failes. Its a composer issue:
"Your requirements could not be resolved to an installable set of packages.
Problem 1
- Installation request for jms/security-extra-bundle 1.5.0 -> satisfiable by jms/security-extra-bundle[1.5.0].
- jms/security-extra-bundle 1.5.0 requires jms/di-extra-bundle 1.3.* -> no matching package found."

its a circular reference problem when running "composer install --dev"

@jaypea
Copy link
Author

jaypea commented Dec 16, 2013

@molchanoviv the factory* properties are integral part of a service definition in symfony. Be it in the PHP Interface, the YML Config or the XML Schema. Therefore they belong to a @service Annotation also.

@jaypea
Copy link
Author

jaypea commented Jan 21, 2014

any news on this?

@jaypea
Copy link
Author

jaypea commented Feb 3, 2014

@schmittjoh ping

@GuilhemN
Copy link
Collaborator

@jaypea can you rebase your PR on master please?

Jan Prieser and others added 5 commits February 16, 2016 16:45
@jaypea jaypea force-pushed the add_missing_service_properties branch from 10d72a5 to f162d22 Compare February 16, 2016 15:55
@jaypea
Copy link
Author

jaypea commented Feb 16, 2016

@Ener-Getick, I just rebased.
thanks for taking care!

@@ -76,6 +80,11 @@ public function serialize()
$this->lookupMethods,
$this->properties,
$this->initMethod,
$this->initMethods,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should add them at the end of the array to avoid BC breaks

@GuilhemN
Copy link
Collaborator

BTW you should wait for #238 otherwise there'll be many conflicts.

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.

6 participants