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

[FrameworkBundle] Remove entities from class map as it is optional #35333

Open
wants to merge 1 commit into
base: 3.4
from

Conversation

@notrix
Copy link

notrix commented Jan 14, 2020

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

This is interesting situation as this is not really a bug fix and not really a new feature.
Not quite sure if it could break something..

But I am certain that this entity import to class map is in the wrong place.
This could be moved to doctrine/doctrine-bundle where it check for orm presence.

My issue comes with PHP 7.4 and class preloading. Project has no doctrine ORM (using ODM instead), but other vendors has entities and those entities are preloaded now. If those entities are missing some dependencies - preloading fails with exceptions. This was not an issue until PHP 7.4, but now I have to work around this forced entity preloading. Why should Framework bundle even care about entities at all?

My temporary solution to this:

class Kernel extends BaseKernel
{
    /**
     * {@inheritDoc}
     */
    public function setAnnotatedClassCache(array $annotatedClasses)
    {
        $filteredArray = array_filter(
            $annotatedClasses,
            function (string $class) {
                return !preg_match("#Entity#", $class);
            }
        );

        parent::setAnnotatedClassCache($filteredArray);
    }
}
@ro0NL
ro0NL approved these changes Jan 14, 2020
@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jan 14, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 14, 2020

This will degrade the performance of apps that rely on cache warmers to be at full speed.
👎 for this reason.

preloading fails with exceptions

preloading should not fail with exception. Without knowing anything else and with a high chance of being wrong, I'd suspect the issue is in the thing that triggers the exception. It shouldn't.

@notrix

This comment has been minimized.

Copy link
Author

notrix commented Jan 15, 2020

preloading should not fail with exception. Without knowing anything else and with a high chance of being wrong, I'd suspect the issue is in the thing that triggers the exception. It shouldn't.

Well I have a bundle that is supporting both Entities and Documents. ORM supporting bundle is optional and it has trait that previous bundle uses. Before PHP 7.4 it was no issue as no one used those entity classes and no one missed this trait, but now entities are actually executed.
Removing entities from class map could lead to performance degradation, but for projects with no orm this could save memory if vendors have classes with word Entity in namespace.

As I mentioned before doctrine/doctrine-bundle extension is checking if orm config is enabled. I think this is the right place to include entities to class map.
Or maybe this could be configurable? With Entities enabled by default.

Any way, in my case always loading to memory classes that I will never use is a performance issue and I will have to work it around.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 20, 2020

This shouldn't consume any more memory past the compilation steps. Can you measure anything?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.