Skip to content

Allow AbstractDoctrineExtension implementations to support the newer bundle structure #43181

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Nov 16, 2021
Merged

Allow AbstractDoctrineExtension implementations to support the newer bundle structure #43181

merged 1 commit into from
Nov 16, 2021

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Sep 26, 2021 •
edited
Loading

Q A
Branch? 5.4
Bug fix? no
New feature? yes
Deprecations? yes
Tickets N/A
License MIT
Doc PR TBD

Container extensions inheriting from Symfony\Bridge\Doctrine\DependencyInjection\AbstractDoctrineExtension (i.e. the extensions in all Doctrine bundles) currently cannot smoothly support the newer bundle directory structure because the existing API relies on information being gleaned from Reflection for the bundle class. This PR is an attempt to allow these extensions to support the newer structure. The notable changes here are:

  • The getMappingDriverBundleConfigDefaults() method adds a string $bundleDir argument which replaces gleaning the bundle directory by reflecting on the Bundle class
  • The abstract getMappingResourceConfigDirectory() method adds a string $bundleDir argument, this is needed for the concrete implementations to have conditional paths supporting both structures (similar to other checks in the framework like the is_dir($bundleDir.'/Resources/views') ? $bundleDir.'/Resources/views' : $bundleDir.'/templates' check for the templates path)

@carsonbot carsonbot added this to the 5.4 milestone Sep 26, 2021
nicolas-grekas
nicolas-grekas reviewed Sep 27, 2021
View reviewed changes
@mbabker mbabker force-pushed the doctrine-extensions-supporting-new-bundle-structure branch from 4f7956f to dcd69c2 Compare September 27, 2021 22:19
@mbabker mbabker changed the title [WIP] Allow AbstractDoctrineExtension implementations to support the newer bundle structure Allow AbstractDoctrineExtension implementations to support the newer bundle structure Sep 27, 2021
@mbabker mbabker changed the title Allow AbstractDoctrineExtension implementations to support the newer bundle structure [DoctrineBridge] Allow AbstractDoctrineExtension implementations to support the newer bundle structure Sep 27, 2021
@mbabker mbabker force-pushed the doctrine-extensions-supporting-new-bundle-structure branch 2 times, most recently from 1abd3c7 to 2dac6d0 Compare October 31, 2021 16:33
@mbabker mbabker force-pushed the doctrine-extensions-supporting-new-bundle-structure branch from 2dac6d0 to 5fb8713 Compare November 13, 2021 17:06
@nicolas-grekas nicolas-grekas force-pushed the doctrine-extensions-supporting-new-bundle-structure branch 2 times, most recently from c321b30 to fe42094 Compare November 13, 2021 17:35
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 13, 2021 •
edited
Loading

@mbabker I force-pushed these changes on your fork, can you please have a look and let me know if that makes sense?

Is that something that can be tested?

@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2021 •
edited
Loading

can you please have a look and let me know if that makes sense?

Looks right to me.

Is that something that can be tested?

Maybe? I'd have to sit down and work out testing the AbstractDoctrineExtension class in general. Right now, I think more of this extension is being tested in the bundles than here.

Edit: I got PhpStorm to run the bridge tests with coverage to see if there was something existing I could add to, loadMappingInformation() has zero coverage right now

@nicolas-grekas
Copy link
Member

I guess this is tested in DoctrineBundle?

@carsonbot carsonbot changed the title [DoctrineBridge] Allow AbstractDoctrineExtension implementations to support the newer bundle structure Allow AbstractDoctrineExtension implementations to support the newer bundle structure Nov 13, 2021
@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2021

It would have to be since loadMappingInformation() gets called when configuring an entity manager.

@nicolas-grekas
Copy link
Member

Can you please send a PR to DoctrineBundle to add the new arguments there?

@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2021

Sure thing!

@mbabker mbabker mentioned this pull request Nov 13, 2021
Merged
@mbabker
Copy link
Contributor Author

mbabker commented Nov 13, 2021

DoctrineBundle PR is doctrine/DoctrineBundle#1418

@mbabker mbabker force-pushed the doctrine-extensions-supporting-new-bundle-structure branch from fe42094 to 6d04a16 Compare November 13, 2021 18:42
@mbabker mbabker mentioned this pull request Nov 13, 2021
Merged
@mbabker mbabker force-pushed the doctrine-extensions-supporting-new-bundle-structure branch from 6d04a16 to 136c4a3 Compare November 13, 2021 19:20
@fabpot
Copy link
Member

fabpot commented Nov 16, 2021

Not sure what "newer bundle directory structure" refers to.
Current docs seem unchanged for many years: https://symfony.com/doc/current/bundles.html

@fabpot
Copy link
Member

fabpot commented Nov 16, 2021

To answer my own question: #32845

@nicolas-grekas
Copy link
Member

See also symfony/symfony-docs#16109, doc has some inconsistencies that should be fixed.

@fabpot
Copy link
Member

fabpot commented Nov 16, 2021

Thank you @mbabker.

@fabpot fabpot mentioned this pull request Nov 16, 2021
Closed
@fabpot fabpot merged commit a0d778e into symfony:5.4 Nov 16, 2021
@mbabker mbabker deleted the doctrine-extensions-supporting-new-bundle-structure branch November 16, 2021 13:05
This was referenced Nov 18, 2021
Merged
Merged
@xabbuh xabbuh mentioned this pull request Nov 30, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabpot fabpot fabpot approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Deprecation Feature Ready Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
4 participants
@mbabker @nicolas-grekas @fabpot @carsonbot