Mark classes as internal that nobody should be depending on. #711

jwage · 2018-05-31T15:18:03Z

Q	A
Type	improvement
BC Break	no
Fixed issues	#710

Summary

Mark classes as internal that nobody should be depending on. The only thing I see that people should be depending on are:

Doctrine\Migrations\Configuration\Configuration
Doctrine\Migrations\Events
Doctrine\Migrations\Finder\MigrationFinder
Doctrine\Migrations\Provider\SchemaProviderInterface
Doctrine\Migrations\Provider\StubSchemaProvider
Doctrine\Migrations\AbstractMigration
- isTransactional
- getDescription
- warnIf
- abortIf
- skipIf
- preUp
- postUp
- preDown
- postDown
- up
- down
- addSql
- write
- throwIrreversibleMigrationException
Doctrine\Migrations\Tools\Console\Command\AbstractCommand
Doctrine\Migrations\Tools\Console\Command\DiffCommand
Doctrine\Migrations\Tools\Console\Command\DumpSchemaCommand
Doctrine\Migrations\Tools\Console\Command\ExecuteCommand
Doctrine\Migrations\Tools\Console\Command\GenerateCommand
Doctrine\Migrations\Tools\Console\Command\LatestCommand
Doctrine\Migrations\Tools\Console\Command\MigrateCommand
Doctrine\Migrations\Tools\Console\Command\RollupCommand
Doctrine\Migrations\Tools\Console\Command\StatusCommand
Doctrine\Migrations\Tools\Console\Command\UpToDateCommand
Doctrine\Migrations\Tools\Console\Command\VersionCommand

jwage · 2018-05-31T15:21:44Z

@Majkl578 @stof Can you take a look? Any classes I marked as internal that shouldn't be?

stof · 2018-05-31T16:11:21Z

lib/Doctrine/Migrations/Configuration/ArrayConfiguration.php

@@ -4,6 +4,9 @@

 namespace Doctrine\Migrations\Configuration;

+/**
+ * @internal


@jwage should these configuration classes really be internal ? How are they used then ?

They don't get used directly, they are a part of the internals of the console commands but are never used directly. They produce a Configuration instance that then gets used in the console commands.

should these classes actually be extending Configuration then ?

@stof arguably no, they should not. A better design would be to inject the Configuration in to ArrayConfiguration, etc. and then call the public getters on Configuration and have no inheritance at all. I could try tackling that refactoring in another PR.

stof · 2018-05-31T16:12:08Z

lib/Doctrine/Migrations/Events.php

@@ -4,6 +4,9 @@

 namespace Doctrine\Migrations;

+/**
+ * @internal


this must not be internal. The events are extension points of the library, so the event names need to be public (otherwise they are not usable).

stof · 2018-05-31T16:12:59Z

lib/Doctrine/Migrations/Finder/MigrationFinder.php

@@ -4,6 +4,9 @@

 namespace Doctrine\Migrations\Finder;

+/**
+ * @internal


Aren't these meant to be an extension point of the library ? Otherwise, how would people choose a different finder ?

Yes, this shouldn't be internal. Changing it.

implementations should not be internal either. Otherwise, the only way to use the extension point is to write your own implementation from scratch, even if you want the same behavior than one of the built-in implementations (as you cannot use them due to being internal and so not covered by BC).

stof · 2018-05-31T16:19:51Z

lib/Doctrine/Migrations/Tools/Console/Helper/MigrationDirectoryHelper.php

@@ -15,6 +15,9 @@
 use function rtrim;
 use function sprintf;

+/**
+ * @internal


console helpers should not be internal either, as they must be registered in the CLI Application too (cli-config.php is expected to instantiate a HelperSet with the appropriate helpers in it)

To be honest I am not sure why this is a symfony console helper. It should not be. It is only used directly here as an internal dependency

migrations/lib/Doctrine/Migrations/Generator/Generator.php

Line 91 in 3105180

$directoryHelper = new MigrationDirectoryHelper($this->configuration);

well, this one should not be a console helper then. But other console helpers should not be internal.

stof · 2018-05-31T16:20:56Z

lib/Doctrine/Migrations/Provider/SchemaDiffProviderInterface.php

@@ -6,6 +6,9 @@

 use Doctrine\DBAL\Schema\Schema;

+/**
+ * @internal


isn't this meant to be an extension point ?

No, only SchemaProviderInterface is an extension point that can be passed through here https://github.com/doctrine/migrations/blob/master/lib/Doctrine/Migrations/Tools/Console/Command/DiffCommand.php

I think the interface might have been created to ease unit testing whenever it was added.

the interface has been added because the lazy one is implemented through decoration

Understood. So I think this should not be public still, right?

yeah, this one is OK. I confused it with SchemaProviderInterface

stof · 2018-05-31T16:50:49Z

lib/Doctrine/Migrations/Provider/OrmSchemaProvider.php

@@ -10,6 +10,9 @@
 use UnexpectedValueException;
 use function count;

+/**
+ * @internal


should not be internal. ORM users should be able to use this implementation of the non-internal SchemaProviderInterface

This gets used internally here. I don't think it should be public https://github.com/doctrine/migrations/blob/master/lib/Doctrine/Migrations/Tools/Console/Command/DiffCommand.php#L136

If they don't provide their own schema provider that implements the SchemaProviderInterface then it uses the OrmSchemaProvider.

sserbin · 2019-01-31T10:32:31Z

@jwage was Doctrine\Migrations\Version\Version accidentally marked as @internal? (it's also not in a list in op)

It seems like it shouldn't have, as Doctrine\Migrations\Configuration::getMigrations() is public (as well as a few others Configuration's methods, accepting or returning Versions )

lcobucci · 2019-01-31T15:09:15Z

@sserbin the implementation has been rightfully marked as @internal. It looks like we forgot to use the interface for those operations, though.

jwage added the Enhancement label May 31, 2018

jwage added this to the 2.0 milestone May 31, 2018

jwage self-assigned this May 31, 2018

jwage requested review from stof and Majkl578 May 31, 2018 15:21

jwage force-pushed the internal-classes branch 2 times, most recently from 254d29a to 84eb10d Compare May 31, 2018 15:50

stof reviewed May 31, 2018

View reviewed changes

jwage force-pushed the internal-classes branch from 84eb10d to fd2428f Compare May 31, 2018 16:44

Mark classes as internal that nobody should be depending on.

970e89f

stof reviewed May 31, 2018

View reviewed changes

jwage force-pushed the internal-classes branch from fd2428f to 970e89f Compare May 31, 2018 16:51

jwage merged commit c2325fd into master May 31, 2018

jwage deleted the internal-classes branch May 31, 2018 18:15

jwage added Improvement and removed Enhancement labels Jun 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mark classes as internal that nobody should be depending on. #711

Mark classes as internal that nobody should be depending on. #711

jwage commented May 31, 2018 •

edited

Loading

jwage commented May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

jwage May 31, 2018

stof May 31, 2018

stof May 31, 2018

jwage May 31, 2018

jwage May 31, 2018

sserbin commented Jan 31, 2019

lcobucci commented Jan 31, 2019

Mark classes as internal that nobody should be depending on. #711

Mark classes as internal that nobody should be depending on. #711

Conversation

jwage commented May 31, 2018 • edited Loading

Summary

jwage commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sserbin commented Jan 31, 2019

lcobucci commented Jan 31, 2019

jwage commented May 31, 2018 •

edited

Loading