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

Add new filters to utils class and change order of meta,terms,media operations in network class. #1095

Merged
merged 24 commits into from
Jul 12, 2023

Conversation

sethrubenstein
Copy link
Contributor

@sethrubenstein sethrubenstein commented Jun 28, 2023

Description of the Change

This adds filters to support #1094 and changes the sequence of operations in push and pull functions of the internal network class.

How to test the Change

  • Attempt distributing a post, everything should work currently as is.

Changelog Entry

Added - dt_before_set_meta, dt_prepared_meta and dt_prepared_taxonomy_terms filter hooks.
Changed - Order of operations setting meta, media and terms in the network push and pull functions.

Credits

Props @sethrubenstein

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. (no new tests needed? funnily enough the existing network class tests list these operations in my desired order)
  • All new and existing tests pass.

@sethrubenstein sethrubenstein requested a review from a team as a code owner June 28, 2023 17:21
@sethrubenstein sethrubenstein requested review from ravinderk and removed request for a team June 28, 2023 17:21
@peterwilsoncc
Copy link
Collaborator

Thanks for the pull request, @sethrubenstein.

I can see the appeal of re-ordering when the media and meta data is processed but am seeing some errors in prepare_meta() while testing. There's a chance it's something I've introduced while working on version 2.0 so want to test a little more before providing a review.

@sethrubenstein
Copy link
Contributor Author

@peterwilsoncc Sounds good, let me know if theres anything you'd like me to change in our PR. Thanks!

@peterwilsoncc peterwilsoncc modified the milestones: 2.0.1, 2.0.0 Jul 6, 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.

Thanks again for the pull request, this change makes senese to the team and we can see the appeal in setting media prior to the meta data.

During testing, I noticed the _thumbnail_id needs to be excluded with the revised order. This prevents set_meta() from undoing the work mapping the image to the new ID in set_media().

I suggest a new method in the NetworkSiteConnection class:

/**
 * Exclude additional meta data for network distributions
 *
 * In network connections the featured image is set prior to the meta data.
 * Excluding the `_thumbnail_id` meta from distribution prevents the meta
 * data from referring to the attachment ID of the original site.
 *
 * @since 2.0.0
 *
 * @param string[] $excluded_meta Array of meta keys to exclude from distribution.
 * @return string[] Array of meta keys to exclude from distribution.
 */
public static function exclude_additional_meta_data( $excluded_meta ) {
	$excluded_meta[] = '_thumbnail_id';
	return $excluded_meta;
}

We are going to try to get this in to Distributor prior to the release of the 2.0.0-beta2 next week (2.0.0-beta1 was released earlier today).

includes/utils.php Outdated Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
includes/utils.php Show resolved Hide resolved
sethrubenstein and others added 3 commits July 6, 2023 22:15
Co-authored-by: Peter Wilson <[email protected]>
Co-authored-by: Peter Wilson <[email protected]>
Co-authored-by: Peter Wilson <[email protected]>
@sethrubenstein
Copy link
Contributor Author

@peterwilsoncc Having some dev env issues at the moment, I should get a chance to double check these changes tomorrow late afternoon.

@sethrubenstein
Copy link
Contributor Author

Alright @peterwilsoncc made some of those updates and implemented a small function with filter to exclude meta keys from distribution in the network class.

@jeffpaul jeffpaul requested a review from peterwilsoncc July 10, 2023 15:34
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 Seth,

I've added a few notes inline.

If you've finished for the day, I can push the changes to your branch if you wish. I'm in Australia so have quite a few hours left in my day.

includes/utils.php Outdated Show resolved Hide resolved
@@ -1203,7 +1249,7 @@ function remote_http_request( $url, $args = array(), $fallback = '', $threshold
/**
* Determines if a post is distributed.
*
* @since 2.0.0
* @since x.x.x
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
* @since x.x.x
* @since 2.0.0

@sethrubenstein
Copy link
Contributor Author

Hi @peterwilsoncc, yeah I'm done w changes on my branch, merged in your comments and brought back the @tutorial snippet to dt_after_set_meta inline comments. Should I add those to other hooks as well or just that one for now?

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, I'll merge in a few docs suggestions and then retest and merge.

includes/utils.php Outdated Show resolved Hide resolved
includes/utils.php Outdated Show resolved Hide resolved
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 Seth, LGTM.

@peterwilsoncc peterwilsoncc merged commit 8eae64c into 10up:develop Jul 12, 2023
@sethrubenstein
Copy link
Contributor Author

@peterwilsoncc Thank you! We're glad to have these new hooks in the plugin.

@peterwilsoncc
Copy link
Collaborator

Thanks for your patience during the code review process @sethrubenstein.

I've just tagged and published the 2.0.0-beta2 release which includes these changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants