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

Avoid deprecation notice on PHP 8.1 due to converting false to array #1040

Merged
merged 2 commits into from
Apr 20, 2023

Conversation

dkotter
Copy link
Collaborator

@dkotter dkotter commented Mar 24, 2023

Description of the Change

In testing something else on Distributor, I was going PHP deprecation notices showing up on the internal Pull Content screen. This notice said: PHP Deprecated: Automatic conversion of false to array is deprecated. Looking at the code, we have a place where a value may be false and then later we use that as an array but we don't ever set that as an array first.

This PR fixes that by adding a check to convert that variable to an array if it isn't already.

How to test the Change

In an environment running PHP 8.1, go to the internal Pull screen and notice a PHP deprecation notice.

Check out the code in this PR and note the notice should no longer be there.

Ensure all connections show up properly in the Pull dropdown, that you can switch between connections successfully, and that you can pull in content.

Changelog Entry

Fixed - Handle a PHP deprecation notice around converting false to array

Credits

Props @dkotter

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@dkotter dkotter self-assigned this Mar 24, 2023
@dkotter dkotter requested a review from a team as a code owner March 24, 2023 20:06
@dkotter dkotter requested review from Sidsector9 and peterwilsoncc and removed request for a team and Sidsector9 March 24, 2023 20:06
peterwilsoncc
peterwilsoncc previously approved these changes Mar 26, 2023
Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM based on a visual review, thanks.

Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a note about the starting state inside the condition.

Push back if I am misunderstanding.

@@ -832,12 +832,13 @@ public static function build_available_authorized_sites( $user_id = false, $cont
$authorized_sites = get_transient( $cache_key );

if ( $force || false === $authorized_sites ) {
$sites = get_sites(
$authorized_sites = ! is_array( $authorized_sites ) ? array() : $authorized_sites;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$authorized_sites = ! is_array( $authorized_sites ) ? array() : $authorized_sites;
$authorized_sites = array();

I think it should always start empty:

  • === false it's not an array
  • $force === true then we're repopulating the authorized sites from 0. If the old value is retained then each site will appear in the array twice.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good point, I think this should always be a new empty array if we make it inside this conditional. I've made that change

Copy link
Collaborator

@peterwilsoncc peterwilsoncc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Darin!

@peterwilsoncc peterwilsoncc merged commit caeb4e7 into develop Apr 20, 2023
@peterwilsoncc peterwilsoncc deleted the fix/convert-false-to-array branch April 20, 2023 23:12
@jeffpaul jeffpaul added this to the 2.0.0 milestone Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants