Best practise for legacy classes

[fritzmg]

As discussed in slack, we should provide a best-practise documentation for the handling of legacy classes. e.g. that they should all reside in a single sub-namespace, like Contao, so that you can easily exclude them from service resource loading like so:

services:
    App\:
        resource: ../src
        exclude: ../src{Contao,DependencyInjection,Resources,Entity}

Background: if you accidentally auto-register a class that extends from \Contao\System, you will run into a fatal error during container building.

[m-vo]

see symfony/symfony#34053

[m-vo]

Services are a concept that works well with composition + interfaces. The legacy code requires you to inherit from all sorts of classes instead. Plus: constructor injection often isn't possible.

Considering this I think a separate sub namespace (that is excluded from service resource loading) for these classes should be the best practice no matter the outcome of the related Symfony issue. The same is true for tests and coverage. I would exclude these classes as well as they aren't testable for the same reason.

So you would end up with App\Contao\Widget\MyWidget instead of App\Widget\MyWidget for example.

[fritzmg]

I would like to get the approval of @contao/developers on this. So in the future, the recommended namespaces would be:

• App\Contao\Model
• App\Contao\Widget
• App\Contao\ContentElement
• App\Contao\FrontendModule

etc.

This would also need to be reflected in the Skeleton Bundle by @leofeyer

[leofeyer]

No, there must not be a Contao namespace.

• App\Model
• App\Widget
• App\ContentElement
• App\FrontendModule

[Toflar]

It depends. I don't think I would like to give a recommendation here. People may still use their brains 😄
If I have an app where I only extend Contao then I would not add this subnamespace. It's useless. If I have an app that consists of content management and an API I would probably make a difference on top level and have additional Contao and Api namespaces.

[fritzmg]

No, there must not be a Contao namespace.

Then the service resource-loading would need to be defined as

services:
    App\:
        resource: ../src
        exclude: ../src{Widget,Model,ContentElement,FrontendModule,DependencyInjection,Resources,Entity}

Plus any other subnamespace of legacy Contao classes.

[aschempp]

• App\ContentElement
• App\FrontendModule

These should be controllers, so either App\Controller or App\Controller\ContentElement accordingly.

[fritzmg]

Of course, this is only in case you absolutely have to use the legacy way.

[aschempp]

Then I agree with @Toflar, there's no recommendation for the old way, because it's outdated 😂
Yeah, people still need it, but there's only a good new way, there's no good old way. So do whatever you feel like …

[fritzmg]

So there is nothing that should be changed in the official documentation and people should deal with problems arising from loading legacy Contao classes as services on their own? Even if symfony/symfony#34053 gets fixed in Symfony 3 & 4, I still think it would be good to show that it is best practise not to load any Contao legacy classes as services.

[m-vo]

Then I agree with @Toflar, there's no recommendation for the old way, because it's outdated joy

Not all things are outdated. How would you create a Widget for example?

[aschempp]

I still think it would be good to show that it is best practise not to load any Contao legacy classes as services.

I think thats a good idea as well. But how's that related to the namespace suggestion you were looking for?

If this is about the automatic service loading, then we should tell people to make sure they only load directories that contain service classes. That might include Doctrine entities, and it includes Contao 3 widgets, or any other class where there are factories for.

[m-vo]

The original issue (undefined constants) has been fixed in the Symfony 3.4 branch.

See symfony/symfony#34560