Skip to content

Conversation

@dero
Copy link
Contributor

@dero dero commented Mar 3, 2025

No description provided.

Copy link
Contributor

@tharsheblows tharsheblows left a comment

Choose a reason for hiding this comment

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

One question about the position of the array check.

}

// Handle unexpected sources variable type.
if ( ! is_array( $sources ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

❓question
Does $sources ever come into this as eg an empty string and then go through and get set via the metadata? Just below this there is a check on the a lot of the metadata to set it where $sources is set.

The check should happen after that I think, from that point on it's assuming $sources is an array

Copy link
Contributor Author

@dero dero Mar 10, 2025

Choose a reason for hiding this comment

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

@tharsheblows Technically, $sources should never be anything else than an array:

The hook clearly documents $sources should always be an array. The intent here was just safeguard against emitting a PHP warning in case something unexpected happens and $sources comes in as a non-array (as it does on one of the customers' site).

Having said that, I do agree with you that it's smarter to move the check after we've inspected post meta as it's entirely possible we'll end up resetting $sources there. Making the change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved in 3fe0646.

@dero dero merged commit f93c53b into develop Mar 11, 2025
4 checks passed
@dero dero deleted the update/media-sources branch March 24, 2025 14:21
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