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

Fix Possible NULL Value of duplicate_post_taxonomies_blacklist #360

Merged
merged 4 commits into from
Mar 22, 2024

Conversation

michael-sumner
Copy link
Contributor

@michael-sumner michael-sumner commented Feb 7, 2024

Context

Cloning a post can cause a critical error due to the type of get_option( 'duplicate_post_taxonomies_blacklist' ) which can be a NULL value.

Related Issues:

  1. https://wordpress.org/support/topic/taxonomies-are-not-copied-because-duplicate_post_taxonomies_blacklist-is-null/
  2. NULL value of duplicate_post_taxonomies_blacklist is not processed correctly #263
  • Fix duplicate_post_taxonomies_blacklist option causing cloning errors due to inconsistent output type (array, empty string, null)

Screenshot 2024-02-07 at 4 50 17 pm

Summary

This PR can be summarized in the following changelog entry:

  • Fixes a bug where a fatal error could be thrown when cloning a post because of a wrong type for the duplicate_post_taxonomies_blacklist option. Props to @michael-sumner.

Relevant technical choices:

  • I added some more checks, reprising some unfinished work that never made it to a PR:
    • covering the case where the value is a string
    • covering also the upgrade routine where the same option is checked

Test instructions

Test instructions for the acceptance test before the PR gets merged

This PR can be acceptance tested by following these steps:

It's a bit tricky to reproduce the problem because it looks like it can be caused by caching, or by pathological situations where the option value has been stored in the wrong way. We'll need to use filters to emulate those cases.

Reproduce the issue with NULL value

  • use a clean env where Duplicate Post has never been active (otherwise, delete all the duplicate_post_* rows in wp_options)
  • install the latest release (4.5)
  • edit your theme's functions.php and add this snippet: add_filter( 'option_duplicate_post_taxonomies_blacklist', '__return_null' );
  • activate Duplicate Post
  • see that you get Fatal error: Uncaught Error: in_array(): Argument #2 ($haystack) must be of type array, null given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 149
  • comment out the line in functions.php
  • visit the post list
  • re-add the line to functions.php
  • choose a post and click on "Clone"
  • see that you get Fatal error: Uncaught Error: array_diff(): Argument #2 must be of type array, null given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 303
  • comment out the line in functions.php before proceeding

Reproduce the issue with a string value

  • use a clean env where Duplicate Post has never been active (otherwise, delete all the duplicate_post_* rows in wp_options)
  • install the latest release (4.5)
  • edit your theme's functions.php and add this snippet: add_filter( 'option_duplicate_post_taxonomies_blacklist', function(){ return 'foo'; } );
  • activate Duplicate Post
  • see that you get Fatal error: Uncaught Error: in_array(): Argument #2 ($haystack) must be of type array, string given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 149
  • comment out the line in functions.php
  • visit the post list
  • re-add the line to functions.php
  • choose a post and click on "Clone"
  • see that you get Fatal error: Uncaught Error: array_diff(): Argument #2 must be of type array, string given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 303
  • comment out the line in functions.php before proceeding

test the fix with NULL value

  • use a clean env where Duplicate Post has never been active (otherwise, delete all the duplicate_post_* rows in wp_options)
  • install this branch
  • edit your theme's functions.php and add this snippet: add_filter( 'option_duplicate_post_taxonomies_blacklist', '__return_null' );
  • activate Duplicate Post
  • see that you don't get Fatal error: Uncaught Error: in_array(): Argument #2 ($haystack) must be of type array, null given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 149
  • visit the post list
  • choose a post that is assigned to one or more categories and click on "Clone"
  • see that you don't get Fatal error: Uncaught Error: array_diff(): Argument #2 must be of type array, null given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 303
  • check that the copy has the same categories applied
  • comment out the line in functions.php before proceeding

test the fix with with a string value

  • use a clean env where Duplicate Post has never been active (otherwise, delete all the duplicate_post_* rows in wp_options)
  • install this branch
  • edit your theme's functions.php and add this snippet: add_filter( 'option_duplicate_post_taxonomies_blacklist', function(){ return 'foo'; } );
  • activate Duplicate Post
  • see that you don't get Fatal error: Uncaught Error: in_array(): Argument #2 ($haystack) must be of type array, string given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 149
  • visit the post list
  • choose a post that is assigned to one or more categories and click on "Clone"
  • see that you don´t get Fatal error: Uncaught Error: array_diff(): Argument #2 must be of type array, string given in /home/lopo/src/Yoast/plugin-development-docker/plugins/duplicate-post/admin-functions.php on line 303
  • check that the copy has the same categories applied

Relevant test scenarios

  • Changes should be tested with the browser console open
  • Changes should be tested on different posts/pages/taxonomies/custom post types/custom taxonomies
  • Changes should be tested on different editors (Default Block/Gutenberg/Classic/Elementor/other)
  • Changes should be tested on different browsers
  • Changes should be tested on multisite

Test instructions for QA when the code is in the RC

  • QA should use the same steps as above.

QA can test this PR by following these steps:

  • N/A

Impact check

This PR affects the following parts of the plugin, which may require extra testing:

  • N/A

UI changes

  • This PR changes the UI in the plugin. I have added the 'UI change' label to this PR.

Documentation

  • I have written documentation for this change. For example, comments in the Relevant technical choices, comments in the code, documentation on Confluence / shared Google Drive / Yoast developer portal, or other.

Quality assurance

  • I have tested this code to the best of my abilities
  • I have added unittests to verify the code works as intended

Innovation

  • No innovation project is applicable for this PR.
  • This PR falls under an innovation project. I have attached the innovation label and noted the work hours.

Fixes #263

@michael-sumner michael-sumner changed the title fix(admin-functions): allow empty or null Fix Possible NULL Value of duplicate_post_taxonomies_blacklist Feb 7, 2024
@enricobattocchi enricobattocchi self-assigned this Feb 8, 2024
@coveralls
Copy link

coveralls commented Mar 22, 2024

Pull Request Test Coverage Report for Build 8390479200

Details

  • 0 of 8 (0.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.08%) to 50.142%

Changes Missing Coverage Covered Lines Changed/Added Lines %
admin-functions.php 0 8 0.0%
Files with Coverage Reduction New Missed Lines %
admin-functions.php 2 0.0%
Totals Coverage Status
Change from base Build 8307438743: -0.08%
Covered Lines: 1237
Relevant Lines: 2467

💛 - Coveralls

@enricobattocchi enricobattocchi added this to the 4.6 milestone Mar 22, 2024
@enricobattocchi enricobattocchi merged commit 9ab8957 into Yoast:trunk Mar 22, 2024
22 checks passed
@enricobattocchi
Copy link
Member

Thanks @michael-sumner! I added some more related fixes that I wasn't able to push before

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.

NULL value of duplicate_post_taxonomies_blacklist is not processed correctly
4 participants