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

Depreciate injection of Request instances #200

Merged
merged 3 commits into from
Jul 25, 2016

Conversation

chalasr
Copy link
Collaborator

@chalasr chalasr commented Jul 18, 2016

Q A
Branch? master
BC breaks? yes (small)
Deprecations? yes
Tests pass? yes

This depreciates the injection of Request instances in order to totally avoid this practice in 2.0. That concerns most of our Event classes.

Warning triggered by Scrutinizer:

Bug
You have injected the Request via parameter $request. This is generally not recommended as there might be multiple instances during a request cycle (f.e. when using sub-requests). Instead, it is recommended to inject the RequestStack and retrieve the current request each time you need it via getCurrentRequest().

The proposed deprecation way consists into:

  • adding a default value (null) for constructors/setters arguments type hinted as Request (BC break)
  • document the corresponding properties+members as deprecated
  • trigger a E_USER_DEPRECATED notice when the deprecated methods are called with a Request as argument.

The recommended alternative is to inject the RequestStack instead.
For Event classes, the user will be responsible of injecting the request stack into their event listeners to be able to use it.

All changed classes are instantiated internally, I think this should not be an hard BC breaking for most people.
Any thoughts?


  • Pre-remove the practice from our code (when RequestStack exists)
  • Documentation needs to be updated with an example of @request_stack injection&usage

Add missing notice-type
Fix wrong message in constructors
Deprecate only for Symfony < 2.8
@chalasr
Copy link
Collaborator Author

chalasr commented Jul 23, 2016

Pending markitosgv/JWTRefreshTokenBundle#45

@chalasr chalasr added this to the 1.7 milestone Jul 23, 2016
@chalasr chalasr merged commit 0906948 into lexik:master Jul 25, 2016
@chalasr chalasr deleted the deprecate_request_arg branch July 25, 2016 22:38
chalasr added a commit to chalasr/LexikJWTAuthenticationBundle that referenced this pull request Jul 25, 2016
 Commits:
    * 0906948 Depreciate injection of Request instances (lexik#200) (@chalasr)

 Conflicts:
	Event/AuthenticationSuccessEvent.php
	Security/Http/Authentication/AuthenticationFailureHandler.php
	Security/Http/Authentication/AuthenticationSuccessHandler.php
	Services/JWTManager.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant