Skip to content

[DoctrineBridge] Add support for a driver type "attribute" #40793

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

Merged
merged 1 commit into from
Apr 16, 2021

Conversation

beberlei
Copy link
Contributor

Q A
Branch? 4.4
Bug fix? yes
New feature? no
License MIT

Without this change its not possible to use attributes for mapping when they get released in ORM 2.9 over the next days. Otherwise we would need to copy three methods from the AbstractDoctrineExtension into the Bundle. See the DoctrineBundle PR that makes the full changes: doctrine/DoctrineBundle#1322

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(once fabbot's patch is applied)

@beberlei beberlei force-pushed the DoctrineBidgeAttributes branch 2 times, most recently from 3a4a581 to a5a14a4 Compare April 13, 2021 09:05
@@ -193,6 +193,10 @@ protected function registerMappingDrivers($objectManager, ContainerBuilder $cont
$args[0] = array_merge(array_values($driverPaths), $args[0]);
}
$mappingDriverDef->setArguments($args);
} elseif ('attribute' === $driverType) {
$mappingDriverDef = new Definition('%'.$this->getObjectManagerElementName('metadata.'.$driverType.'.class%'), [
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure this line is correct?

I suggest to not concat $driverType.

So, should it really be:

new Definition('%'.$this->getObjectManagerElementName('metadata.attribute.class%')

Or did you mean to write:

new Definition('%'.$this->getObjectManagerElementName('metadata.attribute.class').'%'

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I copied that from the annotation code. Yours makes more sense, but the method returns the string , so the result is the same either way. I will clean this up.

@dunglas
Copy link
Member

dunglas commented Apr 13, 2021

This is a duplicate of symfony/doctrine-bridge#10 (but this one looks incomplete).
I missed that this PR wasn't targeting the main repository.

@beberlei beberlei force-pushed the DoctrineBidgeAttributes branch from a5a14a4 to cecaa78 Compare April 16, 2021 13:04
Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@@ -437,7 +441,7 @@ protected function getMetadataDriverClass(string $driverType): string
{
@trigger_error(sprintf('Not declaring the "%s" method in class "%s" is deprecated since Symfony 4.4. This method will be abstract in Symfony 5.0.', __METHOD__, static::class), \E_USER_DEPRECATED);

return '%'.$this->getObjectManagerElementName('metadata.'.$driverType.'.class%');
return '%'.$this->getObjectManagerElementName('metadata.'.$driverType.'.class').'%';
Copy link
Member

Choose a reason for hiding this comment

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

Oh. Thank you

@Nyholm
Copy link
Member

Nyholm commented Apr 16, 2021

👍

@Nyholm Nyholm merged commit 3a021a7 into symfony:4.4 Apr 16, 2021
@beberlei
Copy link
Contributor Author

Thank you everyone, this makes everything easier :)

This was referenced May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants