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 TemplateRegistryAwareInterface #6390

Merged
merged 1 commit into from
Sep 18, 2020

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Sep 15, 2020

Subject

I am targeting this branch, because this change respect BC.

Changelog

### Added
-  `Sonata\AdminBundle\Templating\TemplateRegistryAwareInterface` with `getTemplateRegistry()`, `hasTemplateRegistry()` and `setTemplateRegistry()` methods

@wbloszyk wbloszyk requested a review from a team September 15, 2020 08:53
* @return MutableTemplateRegistryInterface
*/
final public function getTemplateRegistry()
final public function getTemplateRegistry(): ?MutableTemplateRegistryInterface
Copy link
Member

Choose a reason for hiding this comment

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

Why it can return null?

Copy link
Member

Choose a reason for hiding this comment

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

I can in 3.x.

It won't in master. A next major would be needed indeed.

interface TemplateRegistryOwnerInterface
{
// NEXT_MAJOR: uncomment this method in 4.0
//public function getTemplateRegistry(): MutableTemplateRegistryInterface;
Copy link
Member

Choose a reason for hiding this comment

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

And here it cannot?

@@ -1353,7 +1353,7 @@ public function generateMenuUrl($name, array $parameters = [], $referenceType =
return $this->routeGenerator->generateMenuUrl($this, $name, $parameters, $referenceType);
}

final public function setTemplateRegistry(MutableTemplateRegistryInterface $templateRegistry)
final public function setTemplateRegistry(MutableTemplateRegistryInterface $templateRegistry): void
Copy link
Member

Choose a reason for hiding this comment

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

Imo this is a BC break

Copy link
Member

Choose a reason for hiding this comment

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

it's final

@phansys
Copy link
Member

phansys commented Sep 17, 2020

Other packages use the term "aware" instead "owner" for cases like this (ContainerAwareInterface, LoggerAwareInterface, etc).
IMO, based on that premise, we should rename this interface to TemplateRegistryAwareInterface.

@wbloszyk wbloszyk changed the title Add TemplateRegistryOwnerInterface Add TemplateRegistryAwareInterface Sep 17, 2020
@wbloszyk wbloszyk requested review from a team and core23 September 17, 2020 14:31
@wbloszyk wbloszyk merged commit a00155f into sonata-project:3.x Sep 18, 2020
@wbloszyk wbloszyk deleted the add_template_reg_interface branch September 18, 2020 06:51
@VincentLanglet
Copy link
Member

@wbloszyk Please wait for multiple approval before merging (generally 2 is fine).
And if possible, it's generally better to not merge our own PR. ;)

@wbloszyk
Copy link
Member Author

I ask about merge on slack. I prefer 1 approve per refactoring PR and 2+ for new feature.

namespace Sonata\AdminBundle\Templating;

/**
* /**
Copy link
Member

Choose a reason for hiding this comment

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

Typo in comment

@VincentLanglet
Copy link
Member

I ask about merge on slack. I prefer 1 approve per refactoring PR and 2+ for new feature.

It's a team project. And people can't be connected every day.

@greg0ire
Copy link
Contributor

Also, it's open-source software… there are a loooooot more users than a company project.

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

Successfully merging this pull request may close these issues.

None yet

5 participants