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

fix BC break introduced in doctrine 2.6 #3694

Closed
wants to merge 6 commits into
base: develop
from

Conversation

Projects
None yet
2 participants
@danrot
Member

danrot commented Dec 20, 2017

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets ---
Related issues/PRs doctrine/doctrine2#6003
License MIT
Documentation PR ---

What's in this PR?

This PR renames the CollectionRepository::count method to CollectionRepository::countCollection.

Why?

Because starting from Doctrine 2.6 the EntityRepository exposes a count method, which has a different interface than ours. Therefore there is a runtime error thrown (already on composer install)

BC Breaks/Deprecations

The CollectionRepository::count method had to be renamed to CollectionRepository::countCollection.

@danrot danrot referenced this pull request Dec 20, 2017

Merged

keep doctrine 2.5 #3696

@danrot

This comment has been minimized.

Member

danrot commented Dec 20, 2017

It doesn't seem if this is going to work so easily... There even seem to be some error in doctrine itself:

1) Sulu\Bundle\AdminBundle\Controller\AdminControllerTest::testIndexAction
Error: Call to undefined method Doctrine\ORM\Internal\CommitOrderCalculator::addClass()

/Users/daniel.rotter/Development/massiveart/sulu-test/vendor/sulu/sulu/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php:148
/Users/daniel.rotter/Development/massiveart/sulu-test/vendor/sulu/sulu/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Purger/ORMPurger.php:111
/Users/daniel.rotter/Development/massiveart/sulu-test/vendor/sulu/sulu/vendor/doctrine/data-fixtures/lib/Doctrine/Common/DataFixtures/Executor/AbstractExecutor.php:136
/Users/daniel.rotter/Development/massiveart/sulu-test/vendor/sulu/sulu/src/Sulu/Bundle/TestBundle/Testing/SuluTestCase.php:285
/Users/daniel.rotter/Development/massiveart/sulu-test/vendor/sulu/sulu/src/Sulu/Bundle/AdminBundle/Tests/Functional/Controller/AdminControllerTest.php:27
@danrot

This comment has been minimized.

Member

danrot commented Dec 20, 2017

I have created an issue for that: doctrine/data-fixtures#281

@danrot

This comment has been minimized.

Member

danrot commented Dec 20, 2017

I think for now it is better to just stay with doctrine 2.5, made a PR for that: #3696

@danrot

This comment has been minimized.

Member

danrot commented Dec 20, 2017

We've had an outdated version of the data-fixtures package, so I continue to work here.

// see if this bundle uses doctrine orm, and update the schema if so..
// (we assume that the bundle will purge the fixtures etc.)
init_bundle($phpunitFile->getPathname());
write_info('Running tests');
$kernelDirectory = $bundleDir . DIRECTORY_SEPARATOR . get_kernel_dir($phpunitFile->getPathname());
putenv('KERNEL_DIR=' . $kernelDirectory);
$_SERVER['KERNEL_DIR'] = $kernelDirectory;

This comment has been minimized.

@danrot

danrot Dec 20, 2017

Member

That's the solution, we need both lines in order to work correctly with the PHP version conditional in the Symfony Process Component

@danrot danrot added this to the Release 2.0 milestone Dec 20, 2017

@danrot danrot changed the base branch from master to develop Dec 20, 2017

@alexander-schranz

This comment has been minimized.

Member

alexander-schranz commented Dec 20, 2017

The Upgrade to 2.6 has also a Problem as our FieldDescriptor ListBuilder uses the className as aliases which is since 2.6 not longer supported: doctrine/doctrine2#6928

At first I thought we could just replace the aliases with str_replace([':', '//'], '', $alias) but the developer in projects can add conditions to the e.g. DoctrineJoinFieldDescriptor:

              new DoctrineDescriptor(
                    'defaultTranslation',
                    'locale',
                    [
                        'defaultTranslation' => new DoctrineJoinDescriptor(
                            'defaultTranslation',
                            self::$categoryEntityName . '.translations',
                            sprintf('defaultTranslation.locale = %s.defaultLocale', Category::class)
                        ),
                    ]
                )

which will not longer work. And a str_replace over a whole DQL query is not save. At current state we must force 2.5 as we did quickly in #3696

/cc @sulu/core-team

@alexander-schranz alexander-schranz self-requested a review Dec 20, 2017

@alexander-schranz

upgrade to 2.6 not possible currently

@alexander-schranz

This comment has been minimized.

Member

alexander-schranz commented Feb 26, 2018

this was fixed in @wachterjohannes PR or?

@danrot

This comment has been minimized.

Member

danrot commented Feb 28, 2018

Right, closing it

@danrot danrot closed this Feb 28, 2018

@danrot danrot deleted the danrot:hotfix/doctrine-update branch Feb 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment