Don't no-op $user_id in test suite's wp_set_auth_cookie()

[danielbachhuber]

The implementation of wp_set_auth_cookie() in the test suite sets $user_id=null, when it should respect the $user_id passed to the function.

If I pass a user id to wp_set_auth_cookie(), I'd expect the user id to be then passed through to wp_set_auth_cookie()'s actions.

Discovered in ​https://github.com/pantheon-systems/wp-saml-auth/pull/48

From r39219

[danielbachhuber]

@rmccue @jnylen0 Do you recall the origin of $user_id=null?

[jnylen0]

That was just the bare minimum necessary to get the new tests to pass for changing a user's password. I see no problem with changing it as needed.

[johnbillion]

Overriding this function in the test suite is asking for trouble. The pluggable function should be refactored to make it testable.

[jnylen0]

Replying to johnbillion:

Overriding this function in the test suite is asking for trouble.

What kind of trouble would you expect to see?

The pluggable function should be refactored to make it testable.

I think this could be done later, and the simple patch here is probably fine for this ticket - the function is already being overridden in trunk and 4.7.

[danielbachhuber]

Replying to jnylen0:

Replying to johnbillion:

Overriding this function in the test suite is asking for trouble.

What kind of trouble would you expect to see?

I actually ran into the trouble when I opened the issue originally. WP SAML Auth defines wp_set_auth_cookie() in its ​bootstrap.php to overload the behavior of wp_set_auth_cookie() in pluggable.php. When core's test suite began to define wp_set_auth_cookie() too, it caused fatal errors in WP SAML Auth's tests.

I agree with @johnbillion that defining pluggable functions in core's test suite is less than ideal.

The pluggable function should be refactored to make it testable.

I think this could be done later, and the simple patch here is probably fine for this ticket - the function is already being overridden in trunk and 4.7.

We should undo the function being overridden sooner rather than later.

[johnbillion]

39367.diff​ adds a filter to wp_set_auth_cookie() and wp_clear_auth_cookie() which allows the actual output of the cookies to be prevented, while retaining the various logic included with them to run, and the hooks to fire.

Thoughts?

[johnbillion]

Also worth pointing out that the change in [39219] also broke the tests for my User Switching plugin because of the no-oped values in the overridden wp_set_auth_cookie() function.

[jnylen0]

Replying to johnbillion:

Thoughts?

39367.diff​ looks great to me 👍🏻

Do we want to get this change into a point release? Seems like we broke the tests for a few plugins.

[johnbillion]

Just what I was thinking :-)

[johnbillion]

Here's my proposal.

For trunk and the 4.7 branch:

• Remove the overridden wp_set_auth_cookie() and wp_clear_auth_cookie() functions from the test suite.

For the 4.7 branch only:

• Remove the tests which use WP_Test_REST_Users_Controller::verify_user_roundtrip(), as these are the tests which result in the "headers already sent" error unless the auth cookie functions are overridden.

For trunk:

• Implement 39367.diff​.

[johnbillion]

39367.2.diff​ is a patch for the 4.7 branch.

[jnylen0]

@johnbillion I don't think we should put this in 4.7.3 for several reasons.

• It's very late in the cycle - the milestone is clear except for this ticket, and we need to do an RC soon.
• We're making changes to the users endpoint in 4.7.3 (see #39701). While I'm not aware of anything that would break, I'd prefer to leave the related tests in place.
• I'd rather wait a week or two and treat trunk and 4.7 the same way, avoiding divergence between branches and the resulting extra work.

[johnbillion]

Agreed.

[johnbillion]

In 40263:

Build/Test tools: Don't override the wp_set_auth_cookie() and wp_clear_auth_cookie() functions.

Overriding pluggable functions in the test suite is asking for trouble in the future. In addition, it means the test suite can't be guaranteed to behave the same as core.

This instead introduces a send_auth_cookies filter which can be hooked in during the test suite to prevent these functions from attempting to send cookie headers to the client.

Fixes #39367

[johnbillion]

In 40264:

Docs: Remove the duplicate hook documentation for the newly introduced send_auth_cookies filter.

See #39367

[johnbillion]

In 40265:

Build/Test tools: Don't override the wp_set_auth_cookie() and wp_clear_auth_cookie() functions.

Overriding pluggable functions in the test suite is asking for trouble in the future. In addition, it means the test suite can't be guaranteed to behave the same as core.

This instead introduces a send_auth_cookies filter which can be hooked in during the test suite to prevent these functions from attempting to send cookie headers to the client.

Fixes #39367

Merges [40263] and [40264] to the 4.7 branch.

[audrasjb]

In 55164:

Users: Add context to the send_auth_cookies filter.

This changeset adds $user_id, $expire, $expiration and $token parameters to provide context to send_auth_cookies hook, which allows the filter to skip sending auth cookies.

Props dd32, mukesh27, costdev, peterwilsoncc, audrasjb.
Fixes #56971.
See #39367.

[SergeyBiryukov]

In 55165:

Docs: Remove a duplicate line in the send_auth_cookies filter DocBlock.

Describe the default values for the $send and $expire parameters.

Follow-up to [55164].

See #56971, #39367.

[SergeyBiryukov]

In 55166:

Docs: Fix typo in the send_auth_cookies filter DocBlock.

Follow-up to [55164], [55165].

See #56971, #39367.