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

Do not try to register Contao 3 classes as services #1222

Merged
merged 3 commits into from
Jan 27, 2020
Merged

Conversation

aschempp
Copy link
Member

No description provided.

@aschempp aschempp added this to the 4.9 milestone Jan 20, 2020
@aschempp aschempp requested a review from a team January 20, 2020 12:42
@aschempp aschempp self-assigned this Jan 20, 2020
@Toflar
Copy link
Member

Toflar commented Jan 20, 2020

Can you please add a description? I don't understand.

@fritzmg
Copy link
Contributor

fritzmg commented Jan 20, 2020

@Toflar the services.php (or even your own resource loading definition in your services.yml) will register all classes as services in the defined namespace. However, in Contao you still might have things like Models, or old legacy classes that extend from \Contao\System somewhere down the line and are not supposed to be registered as services. Usually you would define these subnamespaces to be excluded in your services.yml. I think @aschempp wants to automatically exclude any class that inherits from \Contao\System to be registered as a service - if they cause an error.

Somewhat related to symfony/symfony#34053

@Toflar
Copy link
Member

Toflar commented Jan 20, 2020

Can't we exclude anything coming from Resources/contao?

@fritzmg
Copy link
Contributor

fritzmg commented Jan 20, 2020

Those classes would not be within Resources, but rather in their own namespace. e.g. App\ContentElement\MyContentElement.

fritzmg
fritzmg previously approved these changes Jan 20, 2020
bytehead
bytehead previously approved these changes Jan 20, 2020
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

Unfortunately, this does not solve the issue. The $definition->hasErrors() is never true in my case. 🤷‍♂

@fritzmg
Copy link
Contributor

fritzmg commented Jan 20, 2020

@leofeyer which error happens?

@leofeyer
Copy link
Member

In DefinitionErrorExceptionPass.php line 54:

Cannot autowire service "App\Content\ContentDownloader":
    argument "$objElement" of method "Contao\ContentElement::__construct()"
    has no type-hint, you should configure its value explicitly.

@aschempp aschempp dismissed stale reviews from bytehead and fritzmg via 14f2cb8 January 24, 2020 18:40
@aschempp
Copy link
Member Author

I have updated the PR. We're now automatically removing any class extending from the legacy framework from autoloading, I would argue that these can/should never be services (if you need them, you can still add them manually).

I'm also removing any class that has a legacy class in the constructor, as these can/should never be services that can be injected.

@leofeyer
Copy link
Member

leofeyer commented Jan 27, 2020

The latest changes fix the problem for me. 👍

@leofeyer leofeyer merged commit 6166557 into 4.9 Jan 27, 2020
@leofeyer leofeyer deleted the bugfix/services branch January 27, 2020 11:50
@leofeyer
Copy link
Member

Thank you @aschempp.

@leofeyer leofeyer changed the title Remove Contao 3 services if they cause errors Do not try to register Contao 3 classes as services Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants