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

[UserManagementBundle][SeoBundle] Deprecate direct container access i… #1998

Merged
merged 1 commit into from Jun 8, 2018

Conversation

deZinc
Copy link
Contributor

@deZinc deZinc commented May 31, 2018

Q A
Bug fix? no
New feature? no
BC breaks? no
Deprecations? yes
Fixed tickets

This PR prepares the code to move away from direct container access in controllers. All $this->get() usages are replaced by $this->container->get() to avoid triggering deprecations in non-user code. In 6.0 we should:

  • remove the get and getParameter overrides
  • extend from the symfony AbstractController
  • override the getSubscribedServices method to extend the list of services to make sure we have all the "default" symfony services and the ones controllers extending the BaseSettingsController need in the service locator. Users extending from this BaseSettingsController should just register their controllers as services.
    • @acrobat thinks that for 6.0 we should support sf4.1+ so we can use the newly introduced "parameter bag" to access the parameters-as-a-service. See symfony/symfony#25288

This is just for controllers extending the base. Separate PRs will handle further container access deprecations.

Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @deZinc, your PR needs some changes

  • It seems that our checklist is missing or incomplete

@deZinc deZinc force-pushed the fix/base-settings-controller branch from a6cf054 to a661658 Compare May 31, 2018 10:04
@Devolicious Devolicious added this to the 5.1.0 milestone Jun 8, 2018
Copy link

@ProfessorKuma ProfessorKuma left a comment

Choose a reason for hiding this comment

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

Hi @deZinc, your PR passed all our requirements.

Thank you for contributing!

@Devolicious Devolicious merged commit d828325 into Kunstmaan:master Jun 8, 2018
Devolicious added a commit that referenced this pull request Jun 8, 2018
* master:
  [AdminBundle] updated aclUpdateCommand to use AclManager (#2001)
  Inject correct services/parameters so our code doesn't trigger deprections (#2006)
  [NodeBundle] Reword deprecation message and provide alternative to the end-user (#2004)
  [AdminBundle][NodeBundle] added new AclManager to handle acl dependency injections (#1999)
  [UserManagementBundle][SeoBundle] Deprecate direct container access in controllers (#1998)
  [AdminBundle][UserManagementBundle] marked service as public using compilerpass (#1997)
  [TranslatorBundle] Fix container deprecations with translation loaders (#2005)
  [KunstmaanNodeSearchBundle]: remove check for class (#2009)
  Select correct root node for multidomain setup and use multidomain config id (#1986)
  [AdminBundle][MultiDomainBundle] deprecated service container usage in domain configuration (#1992)
  [AdminBundle] marked service as public (#1991)
  [AdminBundle] Extend Role from base symfony class (#1981)
  Mark services used in controller as public (#1979)
  Deprecate service container usage in commands (#1974)
  [SearchBundle] Deprecate service container usage in commands (#1978)
  Fix UtilitiesExtensionTest after upmerge from 5.0 (#1977)
  Change labels to required (#1962)
@acrobat acrobat added this to Done in Symfony 4 support Jun 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants