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.

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

Type comment deprecation #628

Closed
wants to merge 2 commits into from
Closed

Type comment deprecation #628

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dc0a355
Add a deprecation notice for this feature
mikeSimonson Feb 22, 2017
0217afa
Changing the default comment option of a type
mikeSimonson Feb 23, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ConnectionFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ private function initializeTypes()
Type::addType($type, $typeConfig['class']);
}
if ($typeConfig['commented']) {
@trigger_error(
'The commented option of the type configuration is deprecated since DoctrineBundle 1.7.' .
'Use the feature of the Type class Instead',
E_USER_DEPRECATED
);
$this->commentedTypes[] = $type;
}
}
Expand Down
2 changes: 1 addition & 1 deletion DependencyInjection/Configuration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ private function addDbalSection(ArrayNodeDefinition $node)
->end()
->children()
->scalarNode('class')->isRequired()->end()
->booleanNode('commented')->defaultTrue()->end()
->booleanNode('commented')->defaultFalse()->end()
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC break...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really a BC break if people can still use the option.
Or should I wait for v2 for this change and v3 to remove the option entirely ?

Copy link
Member

@stof stof Jul 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I suggest a BC way:

  • trigger a deprecation whenever the field is configured explicitly (to allow removal), by using beforeNormalization
  • if the field is configured as commented (either explicitly or due to the default), the DI extension should check whether the type class overwrites the requiresSQLCommentHint method:
    • If no, it should trigger a deprecation saying that types should be marked as commented in the type itself.
    • If yes, it should remove this type from the list of commented types passed to the bundle boot method (actually marking it as not commented in the parameter). Types will still be registered as commented in DBAL due to the type class itself. But it will not trigger early registration anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

For the first part, if it helps, I think this is an example: https://github.com/symfony/symfony/blob/2e47edc4a52c3b61664e512f849c3df5f5c20593/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php#L714-L721

It's checking to see if strict_email is explicitly configured, and it triggers a deprecation warning if it is.

For the second part... I'm not as sure :)

->end()
->end()
->end()
Expand Down
2 changes: 1 addition & 1 deletion Tests/DependencyInjection/AbstractDoctrineExtensionTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,7 +531,7 @@ public function testSetTypes()
$container = $this->loadContainer('dbal_types');

$this->assertEquals(
array('test' => array('class' => TestType::class, 'commented' => true)),
array('test' => array('class' => TestType::class, 'commented' => false)),
Copy link
Member

Choose a reason for hiding this comment

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

This is a BC Break

$container->getParameter('doctrine.dbal.connection_factory.types')
);
$this->assertEquals('%doctrine.dbal.connection_factory.types%', $container->getDefinition('doctrine.dbal.connection_factory')->getArgument(0));
Expand Down