#56971 closed enhancement (fixed)
Add context to send_auth_cookies
Reported by: | dd32 | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Users | Keywords: | has-patch commit |
Focuses: | Cc: |
Description
The send_auth_cookies
filter was introduced in #39367 for unit testing, but it has other benefits in the authentication flow.
However, this filter doesn't pass any context, making it a lot less useful for conditional usage.
The attached PR will add $user_id as context to the filter, which at least allows for the filter to skip sending auth cookies.
Change History (24)
This ticket was mentioned in PR #3554 on WordPress/wordpress-develop by @dd32.
2 years ago
#1
- Keywords has-patch added
#2
@
2 years ago
- Keywords changes-requested added
- Milestone changed from Awaiting Review to 6.2
Thanks @dd32 for ticket and PR. Left one feedback.
Added milestone 6.2
for visibility.
@peterwilsoncc commented on PR #3554:
2 years ago
#3
In instances were these functions are replaced, it's possible the filter will exist with only a single parameter.
Is it worth documenting that developers should account for that somehow when using the second parameter? In all honesty, you probably have a better idea of the history with such changes than I do!
(Please read this as a question rather than a passive aggressive change request. I think you know by now I'd just ask if I thought it needed changes.)
2 years ago
#4
Is it worth documenting that developers should account for that somehow when using the second parameter?
Given it's part of the authentication flow, I would anticipate that anyone who is trying to use it should be aware of validation of parameters. I added the @since
for that purpose.
The same goes for any filter that ever that has had additional parameters be set. So I don't see it as being something that needs to be called out any more than the @since
.
#5
@
22 months ago
- Keywords changes-requested removed
Looks like the changes requested by @mukesh27 have been made. Removing the changes-requested
keyword.
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
22 months ago
22 months ago
#7
Resolved the changes mentioned above in 55989da. If a currently active committer would like to run with this, go for it!
#8
@
22 months ago
- Keywords commit added
Thanks @dd32 for ticket and PR.
This ticket was discussed in the recent bug scrub.
The PR 3554 got enough approval to commit. Adding commit
for consideration
Props to @costdev
#9
@
22 months ago
- Owner set to audrasjb
- Status changed from new to accepted
Thanks everyone, self assigning for commit
.
@audrasjb commented on PR #3554:
22 months ago
#11
Committed in https://core.trac.wordpress.org/changeset/55164
#14
@
22 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Thanks for the commit!
Just thinking, should the order of these new parameters be consistent with that of the set_auth_cookie
and set_logged_in_cookie
actions above? Like this:
if ( ! apply_filters( 'send_auth_cookies', true, $expire, $expiration, $user_id, $scheme, $token ) ) {
instead of this:
if ( ! apply_filters( 'send_auth_cookies', true, $user_id, $expire, $expiration, $token ) ) {
Or is the current order more preferable?
#15
@
22 months ago
- Keywords has-patch commit removed
Thanks for the follow-up commits!
I think the order of the parameters doesn't really matter, so let's make them consistent with other functions
This ticket was mentioned in Slack in #core by mukeshpanchal27. View the logs.
22 months ago
This ticket was mentioned in PR #4017 on WordPress/wordpress-develop by @mukesh27.
22 months ago
#17
- Keywords has-patch added
Trac ticket: https://core.trac.wordpress.org/ticket/56971
#18
@
22 months ago
- Keywords commit added
@mukesh27 submitted PR 4017 to address @audrasjb's feedback. This looks ready for commit
consideration.
@audrasjb commented on PR #4017:
22 months ago
#19
I added another commit to this PR to also change the order in the since mention.
@mukesh27 commented on PR #4017:
22 months ago
#20
Thanks @audrasjb for update.
@audrasjb commented on PR #4017:
22 months ago
#22
committed in https://core.trac.wordpress.org/changeset/55253
Trac ticket: https://core.trac.wordpress.org/ticket/56971