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 support for multiple service annotations #238

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

Conversation

wodka
Copy link
Contributor

@wodka wodka commented Feb 14, 2016

this only adds the ability to define multipe service annotations on a class

to fully implement #232 we need to define how to add injections based on different service notations.

@wodka
Copy link
Contributor Author

wodka commented Feb 14, 2016

perhaps something like:

@Service(
  "first.service", 
  initMethod="setup", 
  initMethodInject=@InjectParams({
    "system" =@Inject("%first%)
  })
)
@Service(
  "second.service", 
  initMethod = "setup", 
  initMethodInject = @InjectParams({
    "system" = @Inject("%second%)
  })
)

Ideas?

*/
public function addService(array $service)
{
if (empty($this->id)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO we should deprecate all this attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the deprecated annotation to old properties

/**
* get list of defined services, use fallback of original fields
*
* return array[]
Copy link
Collaborator

Choose a reason for hiding this comment

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

@return

@wodka wodka force-pushed the service-multiple branch 4 times, most recently from 5275158 to 6379051 Compare February 16, 2016 15:22
add deprecation warnings
add services support to AfterSetup
add tests
fix schmittjoh#232
@wodka
Copy link
Contributor Author

wodka commented Feb 16, 2016

@Ener-Getick I just added the support for AfterSetup services

);
}
foreach ($classMetadata->getServices() as $service) {
if (isset($environment)
Copy link
Collaborator

Choose a reason for hiding this comment

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

always true

Copy link
Collaborator

Choose a reason for hiding this comment

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

Use null !== instead, it is clearer

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed it

@GuilhemN
Copy link
Collaborator

I'm wondering if we should not create a new ServiceMetadata class or just use the Definition class directly...

@GuilhemN
Copy link
Collaborator

@schmittjoh What do you think about having a new metadata class ? something like ServiceMetadata which would contain all the linked fields instead of having an array.

@GuilhemN
Copy link
Collaborator

as this is a big stuff, I think it will wait for 1.8.

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.

2 participants