Skip to content

Enable Controllers without injection (have been missing from ClassHierachy) - #241

Open
wodka wants to merge 3 commits into
schmittjoh:masterfrom
wodka:issue-39
Open

Enable Controllers without injection (have been missing from ClassHierachy)#241
wodka wants to merge 3 commits into
schmittjoh:masterfrom
wodka:issue-39

Conversation

@wodka

@wodka wodka commented Feb 16, 2016

Copy link
Copy Markdown
Contributor

Some controllers might not have an injection defined, those get removed by the metadataFactory because no metadata is preset, but we still need them in the path to have it working

This solves #39

Comment thread HttpKernel/ControllerResolver.php Outdated
* those get remove by the metadataFactory because no metadata is
* preset, but we still need them in the path to have it working
*/
if ($metadata->getOutsideClassMetadata()->name != $class) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's better to use !==

@GuilhemN

Copy link
Copy Markdown
Collaborator

ping @schmittjoh, is this the right fix ?

@schmittjoh

Copy link
Copy Markdown
Owner

Can we track which files we potentially need to require instead?

@wodka

wodka commented Feb 16, 2016

Copy link
Copy Markdown
Contributor Author

I don't know what you mean by require - the issue is that a controller might not have any injections defined.

now the metadata_factory removes all entries if there are no injections defined on that specific class.
-> this in itself is fine for services. therefore we cannot change it there

but now for the controller we need the controller to be the class we check initially -> this is ensured by adding it to the class hierarchy if it's not present

this is a good example for a controller that would create an instance of Controller instead of FooController:
https://github.com/wodka/JMSDiExtraBundle/blob/issue-39/Tests/Functional/Bundle/TestBundle/Controller/Base/FooController.php

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.

3 participants