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

[FEATURE] DBAL 3 #541

Merged
merged 19 commits into from
Feb 16, 2023
Merged

[FEATURE] DBAL 3 #541

merged 19 commits into from
Feb 16, 2023

Conversation

keithbrink
Copy link
Contributor

@keithbrink keithbrink commented Feb 7, 2023

Following on the work of #519 and #520, I went through the code to fix all the breaking changes related to DBAL 3, as well as some deprecations that came up.

I would prefer to target a new 2.0 branch with this, since there are many breaking changes.

  • Removed the Json type since the json_array type has been removed in DBAL 3.
  • Removed the MasterSlaveConnection as it has been removed from DBAL 3.
  • Removed the FluentExporter, GenerateEntitiesCommand, GenerateRepositoriesCommand, ConvertMappingCommand, as in [FEATURE] Upgrading to DBAL 3 & Switch to symfony/cache #520
  • Removed the flush option from the various cache clearing commands; these have been in place since the initial commit to this repository, so I couldn't find any reason why it existed in the first place. (Ideas? @patrickbrouwers) Flush is no longer supported by Symfony cache anyways.
  • Remove the deprecated namespace feature.
  • Updated the PHP CS Fixer file to the new format, using the PSR12 ruleset. Can move this to a separate PR if preferred.
  • Bumped the doctrine/orm version requirement to 2.14 due to the new ORMSetup class and the deprecated create method on the EntityManager: Deprecate EntityManager::create() doctrine/orm#9961

@dpslwk
Copy link
Member

dpslwk commented Feb 7, 2023

Any reason you did no start with #520 ??
We were close on that PR and just a few tweaks requested, was goin to pick it up myself later in the month to finish off

@keithbrink
Copy link
Contributor Author

@dpslwk Didn't see it! I'll have a look at it now.

@keithbrink keithbrink marked this pull request as draft February 7, 2023 21:39
@keithbrink
Copy link
Contributor Author

Working on converting the caches over, marked as draft for now.

@dpslwk
Copy link
Member

dpslwk commented Feb 8, 2023

MasterSlaveConnection was not remove so much as changed to PrimaryReadReplicaConnection
we had a request to make the same changes #520 (comment) in #520

@keithbrink
Copy link
Contributor Author

MasterSlaveConnection was not remove so much as changed to PrimaryReadReplicaConnection we had a request to make the same changes #520 (comment) in #520

Yeah, to be more precise, the new PrimaryReadReplica was already supported in Laravel Doctrine, but it also supported the old MasterSlaveConnection for backwards compatibility. This has now been completely removed from DBAL 3: https://github.com/doctrine/dbal/blob/3.5.x/UPGRADE.md#removed-masterslaveconnection

@codecov-commenter
Copy link

codecov-commenter commented Feb 8, 2023

Codecov Report

Base: 53.56% // Head: 57.94% // Increases project coverage by +4.37% 🎉

Coverage data is based on head (ff3ea4c) compared to base (d2c8bc0).
Patch coverage: 78.78% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@             Coverage Diff              @@
##                2.0     #541      +/-   ##
============================================
+ Coverage     53.56%   57.94%   +4.37%     
+ Complexity      828      660     -168     
============================================
  Files           101       92       -9     
  Lines          2662     2197     -465     
============================================
- Hits           1426     1273     -153     
+ Misses         1236      924     -312     
Impacted Files Coverage Δ
src/Configuration/CustomTypeManager.php 100.00% <ø> (ø)
src/Console/ClearMetadataCacheCommand.php 0.00% <0.00%> (ø)
src/Console/ClearQueryCacheCommand.php 0.00% <0.00%> (ø)
src/Console/ClearResultCacheCommand.php 0.00% <0.00%> (ø)
src/Console/InfoCommand.php 0.00% <ø> (ø)
src/Console/MappingImportCommand.php 0.00% <0.00%> (ø)
src/DoctrineManager.php 85.71% <ø> (-1.25%) ⬇️
src/DoctrineServiceProvider.php 0.00% <ø> (ø)
src/Extensions/ExtensionManager.php 94.59% <ø> (ø)
src/Queue/FailedJobsServiceProvider.php 0.00% <0.00%> (ø)
... and 9 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@keithbrink keithbrink marked this pull request as ready for review February 9, 2023 17:13
@keithbrink
Copy link
Contributor Author

Using this branch within my application and all seems to be good - ready for review.

Copy link
Member

@eigan eigan left a comment

Choose a reason for hiding this comment

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

Seems to look OK to me, just need to test it a bit 👍🏻

phpunit.xml Show resolved Hide resolved
@dpslwk
Copy link
Member

dpslwk commented Feb 9, 2023

Could you add an UPGRADE.md with notes on moving to 2.x version of laravel-doctrine/orm

@keithbrink
Copy link
Contributor Author

Could you add an UPGRADE.md with notes on moving to 2.x version of laravel-doctrine/orm

Done.

UPGRADE.md Outdated Show resolved Hide resolved
src/EntityManagerFactory.php Show resolved Hide resolved
Copy link
Member

@eigan eigan left a comment

Choose a reason for hiding this comment

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

Mention of void cache can be removed from config file.

src/Loggers/Clockwork/DoctrineDataSource.php Outdated Show resolved Hide resolved
@eigan
Copy link
Member

eigan commented Feb 16, 2023

Did some research into the namespaces thing.

  • We are creating a MappingDriverChain with a default driver
  • This default driver is using the namespace LaravelDoctrine
    • The driver is registered as default, which means that it can be used even though the namespace doesn't match an entity classname.
    • The laravel-doctrine/acl package uses this default driver for its own entities (Permission entity). It calls the MappingDriverChain::addPaths().
  • The namespace support we have in orm ^1 works by reusing the same driver for each namespace

Basically the namespace support doesn't seem to have any effect at all. The entities doesn't need to be in any of the namespaces.

@eigan
Copy link
Member

eigan commented Feb 16, 2023

@keithbrink I merged 1.7 into 1.8 and noticed the cs-fixer things was already handled there. Sorry about that 🤦🏻‍♂️

@keithbrink
Copy link
Contributor Author

@eigan No problem, I rebased on 1.8.

@eigan eigan changed the base branch from 1.8 to 2.0 February 16, 2023 19:02
@eigan eigan merged commit 8611491 into laravel-doctrine:2.0 Feb 16, 2023
@eigan
Copy link
Member

eigan commented Feb 16, 2023

@keithbrink Thank you! This is a good starting point for 2.0. Going to update the other laravel-doctrine packages to allow this version soon. Will try to get this out to our staging env tomorrow, then production next friday.

@keithbrink
Copy link
Contributor Author

@eigan Thanks for getting this across the finish line!

@dpslwk dpslwk mentioned this pull request Feb 16, 2023
@eigan eigan mentioned this pull request Feb 17, 2023
Closed
10 tasks
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.

4 participants