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

Add deprecation when injecting registry in command #6420

Merged
merged 1 commit into from
Oct 15, 2020

Conversation

VincentLanglet
Copy link
Member

Subject

I am targeting this branch, because BC.

Some deprecation and some tests updates were missing in #6413

Changelog

### Deprecated
- Deprecate Passing a third argument to `GenerateObjectAclCommand::__construct()`

@VincentLanglet VincentLanglet requested review from franmomu and a team September 27, 2020 00:47
@franmomu
Copy link
Member

The problem I see with this is that as it is, is that it will throw an unavoidable deprecation because doctrine is still being injected:

(new ReferenceConfigurator('doctrine'))->nullOnInvalid(),

If we don't pass it, it will be BC-break (because the user_model won't work). So maybe we can create an @internal setter (to be removed in next major) and call it in the definition and don't pass the $registry in the constructor.

@VincentLanglet
Copy link
Member Author

So maybe we can create an @internal setter (to be removed in next major) and call it in the definition and don't pass the $registry in the constructor.

Sure. Could you give me the code to write in the commands.php ? @franmomu

@VincentLanglet VincentLanglet force-pushed the deprecateRegistry branch 2 times, most recently from 61f8b95 to e7567df Compare September 27, 2020 08:25
@franmomu
Copy link
Member

I guess this would work:

->set(GenerateObjectAclCommand::class, GenerateObjectAclCommand::class)
    ->public()
    ->tag('console.command')
    ->args([
        new ReferenceConfigurator('sonata.admin.pool'),
        []
    ])
    ->call('setRegistry', [(new ReferenceConfigurator('doctrine'))->nullOnInvalid()])

@VincentLanglet VincentLanglet force-pushed the deprecateRegistry branch 2 times, most recently from 5957e5e to 04e3352 Compare September 27, 2020 08:40
@VincentLanglet
Copy link
Member Author

Thanks @franmomu, I've made the changes

@SonataCI
Copy link
Collaborator

Could you please rebase your PR and fix merge conflicts?

@VincentLanglet VincentLanglet force-pushed the deprecateRegistry branch 2 times, most recently from 0c53e54 to cae9643 Compare October 5, 2020 21:56
franmomu
franmomu previously approved these changes Oct 5, 2020
*
* NEXT_MAJOR: Remove this class
*/
class DeprecatedGenerateObjectAclCommandTest extends TestCase
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class DeprecatedGenerateObjectAclCommandTest extends TestCase
final class DeprecatedGenerateObjectAclCommandTest extends TestCase

OskarStark
OskarStark previously approved these changes Oct 7, 2020
Copy link
Member

@OskarStark OskarStark left a comment

Choose a reason for hiding this comment

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

Just a minor comment

@VincentLanglet
Copy link
Member Author

I made the requested change @OskarStark

@VincentLanglet VincentLanglet merged commit 8b56291 into sonata-project:3.x Oct 15, 2020
@VincentLanglet VincentLanglet deleted the deprecateRegistry branch October 15, 2020 15:19
$pool = new Pool($this->container, '', '');

$registry = $this->createStub(RegistryInterface::class);
$this->expectDeprecation('Passing a third argument to %s() is deprecated since sonata-project/admin-bundle 3.x.');
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that normal? If not, why did the test pass? They don't pass when trying to merge this up.

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, %s() is weird... I think I have made a mistake

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants