Drop helperset usage in favor of the DependencyFactory#898
Drop helperset usage in favor of the DependencyFactory#898greg0ire merged 18 commits intodoctrine:masterfrom
Conversation
lib/Doctrine/Migrations/Configuration/ConfigurationFormatLoader.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/Migrations/Configuration/Connection/ConfiguredConnectionLoader.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/Migrations/Tools/Console/Command/DoctrineCommand.php
Outdated
Show resolved
Hide resolved
goetas
force-pushed
the
remove-helperset-usage
branch
4 times, most recently
from
f4e8975 to
dd84fbd
Compare
|
this is ready for review |
goetas
force-pushed
the
remove-helperset-usage
branch
2 times, most recently
from
f038112 to
ece01e0
Compare
| declare(strict_types=1); | ||
|
|
||
| namespace Doctrine\Migrations\Configuration\Exception; | ||
| namespace Doctrine\Migrations\Configuration\Configuration\Exception; |
There was a problem hiding this comment.
Is this on purpose? I find the double Configuration in the namespace rather strange.
There was a problem hiding this comment.
It is intentional. I'm also not very happy, but was done to give come structure. Currently there are 3 types of configurations: Migration configs, Connection configs and Entity manager configs. So the idea was to keep all of them in the same folder, but in 3 subfolders.
If you have another idea, will be happy to change it.
There was a problem hiding this comment.
How about Doctrine\Migrations\Configuration\(EntityManager|Connection|Migration)? Configuration\Configuration seems odd, and doesn't convey why we need the additional subnamespace.
lib/Doctrine/Migrations/Configuration/Configuration/Exception/InvalidConfigurationFormat.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/Migrations/Configuration/Configuration/Exception/MissingConfigurationFile.php
Show resolved
Hide resolved
lib/Doctrine/Migrations/Configuration/Connection/ConfigurationFile.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/Migrations/Configuration/Connection/ExistingConnection.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/Migrations/Configuration/EntityManager/ConfigurationFile.php
Outdated
Show resolved
Hide resolved
lib/Doctrine/Migrations/Configuration/EntityManager/ExistingEntityManager.php
Outdated
Show resolved
Hide resolved
goetas
force-pushed
the
remove-helperset-usage
branch
from
3d4ffea to
1f96b53
Compare
lib/Doctrine/Migrations/Configuration/Configuration/Exception/InvalidConfigurationFormat.php
Outdated
Show resolved
Hide resolved
tests/Doctrine/Migrations/Tests/Tools/Console/Command/LatestCommandTest.php
Outdated
Show resolved
Hide resolved
tests/Doctrine/Migrations/Tests/Tools/Console/Command/MigrationVersionTest.php
Outdated
Show resolved
Hide resolved
tests/Doctrine/Migrations/Tests/Tools/Console/Command/StatusCommandTest.php
Outdated
Show resolved
Hide resolved
tests/Doctrine/Migrations/Tests/Tools/Console/Command/UpToDateCommandTest.php
Outdated
Show resolved
Hide resolved
tests/Doctrine/Migrations/Tests/Tools/Console/ConsoleRunnerTest.php
Outdated
Show resolved
Hide resolved
Co-Authored-By: Grégoire Paris <postmaster@greg0ire.fr>
Co-Authored-By: Grégoire Paris <postmaster@greg0ire.fr>
goetas
force-pushed
the
remove-helperset-usage
branch
from
8c05a66 to
442c9a8
Compare
|
@greg0ire all your comments have been addressed except the one where the conversation is not marked as resolved. |
alcaeus
left a comment
alcaeus
left a comment
There was a problem hiding this comment.
Still not happy about the Configuration\Configuration namespace bit, but feel free to create an issue for that and refactor it in a separate PR if you want to work on other things first. Rest LGTM!
|
Thanks @goetas ! |
4 participants
Summary
Even if symfony/symfony#27647 did not pass, it is a good idea to not depend on the symfony helperset feature. The doctrine's DependencyFactory is a better way to get command dependencies.