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

Drop unnesessary constructor #701

Merged
merged 1 commit into from
May 8, 2020

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented May 5, 2020

Subject

This change will allow prepare blocks which extends AbstractAdminBlockService to use one parameter constructor required in BlockBundle 4.x

I am targeting this branch, because it is PATCH.

/**
* @param string $name
*/
public function __construct($name, EngineInterface $templating)
Copy link
Member

Choose a reason for hiding this comment

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

The only change I see at first glance is that parent definition is allowing null for the 2nd argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

abstract class AbstractAdminBlockService extends AbstractBlockService implements AdminBlockServiceInterface

abstract class AbstractBlockService implements BlockServiceInterface
{
    ...

    /**
     * NEXT_MAJOR: Make `$twig` argument mandatory and remove other arguments.
     *
     * @param Environment|EngineInterface|string $templatingOrDeprecatedName
     */
    public function __construct($templatingOrDeprecatedName = null, EngineInterface $templating = null)
    ...

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's exactly what I'm pointing (EngineInterface $templating = null). Although I don't know if it worths.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change is require to #700

@wbloszyk wbloszyk force-pushed the fix_unnecessary_constructor branch from f676e41 to 22d99a9 Compare May 5, 2020 13:47
@wbloszyk wbloszyk requested a review from phansys May 5, 2020 13:48
@greg0ire greg0ire merged commit 7dd12dd into sonata-project:3.x May 8, 2020
@greg0ire
Copy link
Contributor

greg0ire commented May 8, 2020

Thanks @wbloszyk !

@wbloszyk
Copy link
Member Author

wbloszyk commented May 8, 2020

@greg0ire Can you release it? I need it in sonata-project/SonataMediaBundle#1740

@wbloszyk wbloszyk deleted the fix_unnecessary_constructor branch May 8, 2020 08:22
@greg0ire
Copy link
Contributor

greg0ire commented May 8, 2020

Sure!

@wbloszyk
Copy link
Member Author

wbloszyk commented May 8, 2020

Sure!

@greg0ire Can you edit AbstractAdminBlockService and fix constructor too?

    public function __construct($name, EngineInterface $templating)

to

    public function __construct($name, ?EngineInterface $templating = null)

? is missing too :(

My bad, i miss it.

@greg0ire
Copy link
Contributor

greg0ire commented May 8, 2020

There is no constructor in AbstractAdminBlockService, because you removed it in this PR

@wbloszyk
Copy link
Member Author

wbloszyk commented May 8, 2020

Oh, this PR fix it. I dont have it in media-bundle becouse it is not release yet. 🥇 for me. All it is OK.

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