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

Avoid Connection error when calling ClassMetadataFactor::getAllMetadata() #1294

Merged

Conversation

weaverryan
Copy link
Contributor

Hi guys!

When you pair the ORM with DBAL 2.5.0, then getting the targetPlatform means that a connection will be made to the database. For that reason, it's really important to not get the targetPlatform unless it's absolutely needed. Currently, if you call ClassMetadataFactor::getAllMetadata(), it will try to determine targetPlatform (in initialize()), which will make a connection. And if that connection fails (e.g. no db yet), it will blow up - even though getAllMetadata() doesn't need the targetPlatform.

This fixes that, and in an absolutely BC way, since targetPlatform is private (yay!). This should fix a number of issues in userland, like symfony/symfony-standard#748 and symfony/symfony-standard#774.

This is a PR to master (per the guidelines), but the real target is 2.4, since it's the latest stable. The patch won't apply cleanly, but it's simple: remove from initialize, then replace all references to the new private method.

Thanks in advance! More details are on the commit message.

… errors

initialize() is called sometimes, even when the following code doesn't need
the targetPlatform property. Specifically, in AbstractClassMetadataFactory::getAllMetadata().

But as of DBAL 2.5.0, calling Connection::getDatabasePlatform() will make a
connection to the database, which means that sometimes it may fail (e.g. you
haven't configured your database yet). As a result, calling a method like
AbstractClassMetadataFactory::getAllMetadata() - which does not need the
targetPlatform - will fail, because determining the targetPlatform requires
a connection, which fails.

This avoids that - we only get the targetPlatform *when* we need it, which
are cases where we're doing things that do indeed need a connection.
@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3551

We use Jira to track the state of pull requests and the versions they got
included in.

@fabpot
Copy link
Member

fabpot commented Jan 30, 2015

👍


private function getTargetPlatform()
{
if ($this->targetPlatform === null) {
Copy link
Member

Choose a reason for hiding this comment

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

if ( ! $this->targetPlatform) {

@Ocramius
Copy link
Member

This looks good to me, but it needs a test case to prevent regressions. The test should probably simply use a fake connection object that throws an exception or such...

@Ocramius
Copy link
Member

Another issue that arises from this is that mapping information is not independent from the platform: that may be a problem in future...

@deeky666
Copy link
Member

@Ocramius well that is not particularly an issue with this PR, is it? And IMO never was independent before by asking the platform for its preferences like identity over sequence generator etc. I would even say it cannot be independent at all because of differences in supported features of each platform. Or am I on the wrong track here?
Patch looks good to me, too. Mergie after test case...

@@ -753,4 +752,13 @@ protected function isEntity(ClassMetadataInterface $class)
{
return isset($class->isMappedSuperclass) && $class->isMappedSuperclass === false;
}

private function getTargetPlatform()
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a simple DocBlock here with return type, please? Would help the IDE I suppose...

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

well that is not particularly an issue with this PR, is it?

No, it is indeed out of the scope of this PR. I'm just seeing more flaws in the current implementation, and simply shouting them out for awareness.

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

I would even say it cannot be independent at all because of differences in supported features of each platform. Or am I on the wrong track here?

I think we have a general metadata problem here, as we are storing platform specific information in metadata that may be cached and reused across multiple connections.

@weaverryan
Copy link
Contributor Author

Thanks guys - I've updated the PR with one small change that was suggested and.... a test! I verified that the test fails as expected before this patch, then passes afterwards.

Thanks for the attention!

{
$driverMock = new DriverMock();
$config = new \Doctrine\ORM\Configuration();
$config->setProxyDir(__DIR__ . '/../../Proxies');
$config->setProxyNamespace('Doctrine\Tests\Proxies');
$eventManager = new EventManager();
$conn = new ConnectionMock(array(), $driverMock, $config, $eventManager);
$mockDriver = new MetadataDriverMock();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This $mockDriver was never actually used in this function. So while it's unrelated to my change, I removed it.

@guilhermeblanco
Copy link
Member

👍

guilhermeblanco added a commit that referenced this pull request Feb 4, 2015
Avoid Connection error when calling ClassMetadataFactor::getAllMetadata()
@guilhermeblanco guilhermeblanco merged commit 4c68a38 into doctrine:master Feb 4, 2015
@weaverryan
Copy link
Contributor Author

Thank you @guilhermeblanco! I don't know your process, so will/how can this be backported to 2.4? I know it won't patch cleanly (due to some refactoring inside this class), but in thought, the idea patches fine backwards. I can open a PR to the 2.4 branch if it saves someone time.

Thanks!

@Ocramius
Copy link
Member

Ocramius commented Feb 4, 2015

I'll backport it

Ocramius added a commit that referenced this pull request Feb 4, 2015
Ocramius added a commit that referenced this pull request Feb 4, 2015
@Ocramius
Copy link
Member

Ocramius commented Feb 4, 2015

Merged back into 2.4 at e05930e

@weaverryan
Copy link
Contributor Author

Thank you!

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.

None yet

6 participants