fix(reader-activation): return all labels when no key is requested#4765
Closed
thisismyurl wants to merge 1 commit into
Closed
fix(reader-activation): return all labels when no key is requested#4765thisismyurl wants to merge 1 commit into
thisismyurl wants to merge 1 commit into
Conversation
…utomattic#4560) get_reader_activation_labels() used its own $key parameter as the foreach loop variable while building the labels, so after the loop $key held the last label key instead of null. A no-argument call then failed the `if ( ! $key )` check and returned a single label string instead of the full labels array, which is what wp_localize_script() passes to the auth script. Rename the loop variable so the parameter is preserved. Adds a regression test that invokes the getter with no key and asserts it returns the full array.
|
👋 Thanks for your interest in contributing to Newspack! Newspack development has moved to a single monorepo: Automattic/newspack-workspace. This repository is now a read-only mirror, so we're automatically closing new pull requests here. Please reopen your change against the monorepo – |
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a Reader Activation label retrieval bug where calling get_reader_activation_labels() with no key could incorrectly return a single string (breaking wp_localize_script() usage) due to variable shadowing inside the label-building foreach.
Changes:
- Rename the
foreachloop key variable inReader_Activation::get_reader_activation_labels()to avoid overwriting the method parameter. - Add a PHPUnit regression test ensuring a no-key call returns the full labels array.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| includes/reader-activation/class-reader-activation.php | Prevents $key parameter shadowing so no-arg calls correctly return the full labels array. |
| tests/unit-tests/reader-activation.php | Adds regression coverage for the no-key labels getter behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+43
to
+61
| public function test_get_reader_activation_labels_returns_full_array() { | ||
| $reflection = new \ReflectionClass( Reader_Activation::class ); | ||
|
|
||
| // Reset the cached labels so the label-building loop runs on this call. | ||
| $cache = $reflection->getProperty( 'reader_activation_labels' ); | ||
| $cache->setAccessible( true ); | ||
| $cache->setValue( null, [] ); | ||
|
|
||
| $method = $reflection->getMethod( 'get_reader_activation_labels' ); | ||
| $method->setAccessible( true ); | ||
|
|
||
| $labels = $method->invoke( null ); | ||
|
|
||
| $this->assertIsArray( $labels, 'A no-key call must return the full labels array, not a single label.' ); | ||
| $this->assertArrayHasKey( 'account_link', $labels ); | ||
|
|
||
| // Leave no static residue for later tests in this process. | ||
| $cache->setValue( null, [] ); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey folks,
Calling
get_reader_activation_labels()with no key returns a single label string instead of the full labels array. The label-building loop reuses the method's own$keyparameter as itsforeachvariable, so by the time the loop ends$keyholds the last label key rather than null. Theif ( ! $key )guard then falls through and returns one label. Since that array is whatwp_localize_script()hands to the auth script, the front end receives a broken labels object.Renaming the loop variable is the whole fix. I added a test that calls the getter with no key and asserts it returns the full array; it resets the static label cache afterward so it leaves no residue for other tests in the process.
To verify by hand: register a filter on
newspack_reader_activation_auth_labels, load a page that enqueues the auth script, and confirm the localizednewspack_reader_activation_labelsis the full object rather than a single string. Or runtest_get_reader_activation_labels_returns_full_array. I proved the logic with a standalone harness (a no-key call returns a string without the fix, the full array with it); I don't have a local wp-env, so the PHPUnit case is written to match the existing tests and runs on CI.Closes #4560
(full disclosure: AI helped me identify the issue and verify my work)