Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@mbabker
Copy link
Contributor

@mbabkermbabker commentedSep 26, 2021•
edited
Loading

QA
Branch?5.4
Bug fix?no
New feature?yes
Deprecations?yes
TicketsN/A
LicenseMIT
Doc PRTBD

Container extensions inheriting fromSymfony\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:

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

IonBazan reacted with thumbs up emoji
@mbabkermbabkerforce-pushed thedoctrine-extensions-supporting-new-bundle-structure branch from4f7956f todcd69c2CompareSeptember 27, 2021 22:19
@mbabkermbabker changed the title[WIP] Allow AbstractDoctrineExtension implementations to support the newer bundle structureAllow AbstractDoctrineExtension implementations to support the newer bundle structureSep 27, 2021
@mbabkermbabker changed the titleAllow AbstractDoctrineExtension implementations to support the newer bundle structure[DoctrineBridge] Allow AbstractDoctrineExtension implementations to support the newer bundle structureSep 27, 2021
@mbabkermbabkerforce-pushed thedoctrine-extensions-supporting-new-bundle-structure branch 2 times, most recently from1abd3c7 to2dac6d0CompareOctober 31, 2021 16:33
@mbabkermbabkerforce-pushed thedoctrine-extensions-supporting-new-bundle-structure branch from2dac6d0 to5fb8713CompareNovember 13, 2021 17:06
@nicolas-grekasnicolas-grekasforce-pushed thedoctrine-extensions-supporting-new-bundle-structure branch 2 times, most recently fromc321b30 tofe42094CompareNovember 13, 2021 17:35
@nicolas-grekas
Copy link
Member

nicolas-grekas commentedNov 13, 2021•
edited
Loading

@mbabker I force-pushedthese 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
ContributorAuthor

mbabker commentedNov 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 theAbstractDoctrineExtension 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?

@carsonbotcarsonbot changed the title[DoctrineBridge] Allow AbstractDoctrineExtension implementations to support the newer bundle structureAllow AbstractDoctrineExtension implementations to support the newer bundle structureNov 13, 2021
@mbabker
Copy link
ContributorAuthor

It would have to be sinceloadMappingInformation() 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
ContributorAuthor

Sure thing!

@mbabker
Copy link
ContributorAuthor

DoctrineBundle PR isdoctrine/DoctrineBundle#1418

@mbabkermbabkerforce-pushed thedoctrine-extensions-supporting-new-bundle-structure branch from6d04a16 to136c4a3CompareNovember 13, 2021 19:20
@fabpot
Copy link
Member

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

To answer my own question:#32845

mbabker reacted with laugh emoji

@nicolas-grekas
Copy link
Member

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

@fabpot
Copy link
Member

Thank you@mbabker.

@fabpotfabpot merged commita0d778e intosymfony:5.4Nov 16, 2021
@mbabkermbabker deleted the doctrine-extensions-supporting-new-bundle-structure branchNovember 16, 2021 13:05
This was referencedNov 18, 2021
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

4 participants

@mbabker@nicolas-grekas@fabpot@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp