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

Use filesystem for copying media when doing a network pull/push #567

Merged

Conversation

petenelson
Copy link
Contributor

Description of the Change

Use filesystem for copying media when doing a network pull/push instead of download_url()

Benefits

For network/multisite on the same server, there should be no need to make an HTTP request to download media when doing a pull or push. In some instances, due to DNS resolution, firewalls, HTTP authentication, etc, downloading media via a URL may fail. This update uses the filesystem to copy media rather than downloading it.

Possible Drawbacks

None that I am aware of.

Verification Process

Added some debugging code to save dt_original_file_path to post meta that is only updated when a file is copied successfully. Performed both multisite pushes and pulls to confirm the media is copied.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Applicable Issues

#380

Changelog Entry

  • Use filesystem for copying media when doing a network pull/push instead of download_url()

@jeffpaul jeffpaul requested a review from dinhtungdu March 19, 2020 20:16
@jeffpaul jeffpaul added this to the 2.0.0 milestone Mar 19, 2020
@jeffpaul jeffpaul added the type:bug Something isn't working. label Mar 19, 2020
@@ -144,7 +144,7 @@ public function push( $post_id, $args = array() ) {
* @return {bool} If Distributor should push the post media.
*/
if ( apply_filters( 'dt_push_post_media', true, $new_post_id, $media, $post_id, $args, $this ) ) {
\Distributor\Utils\set_media( $new_post_id, $media );
\Distributor\Utils\set_media( $new_post_id, $media, [ 'use_filesystem' => true ] );
Copy link
Contributor

Choose a reason for hiding this comment

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

The PR works in my test. But I think we only use filesystem for network connection, so we can use DT_IS_NETWORK to detect multisite installations without introducing a new argument for set_media function.

Copy link
Contributor

Choose a reason for hiding this comment

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

@petenelson What do you think about my comment above? (Forgot to tag you 😅)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@dinhtungdu So I think the only issue using that constant here is that is only set to true if we are on multisite and if Distributor is network activated. So there's a chance someone could be using multisite but not have the plugin network activated, just activated on the individual sites they want to use. In that case, using that constant wouldn't work

@jeffpaul
Copy link
Member

As there's still a requested change on this PR, I'm going to move this to our next release so we can clear the 1.6 milestone and get to a release.

@jeffpaul jeffpaul modified the milestones: 1.6.0, 2.0.0 Apr 16, 2020
@dkotter
Copy link
Collaborator

dkotter commented May 15, 2020

Made some minor formatting changes and fixed the unit tests. This works well in my tests 👍

@jeffpaul jeffpaul modified the milestones: 2.0.0, 1.6.0 May 15, 2020
@jeffpaul jeffpaul merged commit 26f7850 into 10up:develop May 15, 2020
@iaingooey
Copy link

Hi There, Is there any chance of 2.1 coming out? just regarding the Featured images not pulling through to duplicate sites?

Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug Something isn't working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants