support exporting attachments when specific post ids are requested #16

drzraf · 2017-08-29T16:59:06Z

support exporting attachments when specific post ids are requested (assuming the post whose ids were specified were not attachment themselves)

getenv() to limit merge-conflict with other pending PR + fact that there may be long debates about how to pass the option, eg: --attachments / --force-attachment / --post__in=+<num>,-<num>,... / --with-deps / ...

drzraf · 2017-08-29T17:01:44Z

About the change in attachments_for_specific_post_types(). This function should be made more generic (accept an array of filters?)
On overall, reusing WP_Query() may be a good choice to not duplicate query-builder logic?

schlessera · 2017-08-30T06:09:21Z

Using getenv() is not a good choice here. All options for commands should be consistent and have proper command line arguments. Otherwise, we'll end up with countless custom constants over time.

In general, the process should be to open an issue first describing the problem that you're trying to solve, and then we can discuss details like the names of the arguments together, before you spend time writing code. See the section about the pull request workflow in the handbook.

I think going with a --attachments flag is pretty straightforward, provided it mimics the current behaviour.

So, the flag should default to true, unless you specify specific post IDs with --post__in, in which case it should default to false.

This will not only allow what you have proposed above, but will also allow to disable attachments for any of the other use case if needed.

What do you think?

danielbachhuber · 2017-08-30T12:06:39Z

I think going with a --attachments flag is pretty straightforward, provided it mimics the current behaviour.

One nit: --attachments is ambiguous. Something like --with-attachments is better, but still doesn't describe the fact that attachments are always included by default.

danielbachhuber · 2017-08-30T12:09:03Z

src/WP_Export_Query.php

@@ -262,7 +266,7 @@ private function category_where() {

 	private function attachments_for_specific_post_types( $post_ids ) {
 		global $wpdb;
-		if ( !$this->filters['post_type'] ) {


In the WordPress admin, exporting a specific post type doesn't include attachments. We need to persist the current behavior here. If the --with-attachments flag is included, then we can include the attachments.

drzraf · 2017-08-30T16:50:57Z

> @@ -262,7 +266,7 @@ private function category_where() { private function attachments_for_specific_post_types( $post_ids ) { global $wpdb; - if ( !$this->filters['post_type'] ) {

I'd expect this hunk to cause trouble. But I'd rather let you decide the future for this function and the way this must be changed (if it really has to)

drzraf · 2017-08-31T03:52:02Z

Thanks for the feedbacks about --(with-)attachments

PR #13 is 2 weeks old, all its issues have been addressed (except an inoffensive wp_clear_object_cache() that any maintainer may fix himself in order to merge (or fix later if it really needs to))
I'm really not willing to maintain many feature-branches and/or fighting to introduce features in a conflict-less way with other pending patches (and even less willing to rebase)

For that reason the getenv() to limit merge-conflict with other pending PR
Although rerolls are sometimes really needed, many others about typo/details/... would really have have to be non-merge-blocking in order to ease fruitful contribution.
(and note the 10 minutes runs of the testsuite)

I'd really loved if you, knowledgeable maintainers, responsible of codebase quality, could manage the 5 or 10% left for a PR rather than expecting outside contributor to reroll the patch X times to reach a perfect the 100% quality expected. Would make wp-cli contribution experience much less frustrating for the best of the software.

For the context, I'm working on a plugin + continuous integration (to ensure code quality and avoid regression). wp-cli is just a tool for that end and I intend to make it more performant for the task in the hope it benefit to other WP dev, but I currently can't afford dozens of hours to wp-cli itself.

danielbachhuber · 2017-08-31T11:36:50Z

I'd really loved if you, knowledgeable maintainers, responsible of codebase quality, could manage the 5 or 10% left for a PR rather than expecting outside contributor to reroll the patch X times to reach a perfect the 100% quality expected. Would make wp-cli contribution experience much less frustrating for the best of the software.

All you need to do is ask :)

drzraf · 2017-09-01T21:43:06Z

thx for the positive reply @danielbachhuber
Added a commit using --with_attachments and addressing review of @schlessera
For time constraint and given that wp post generate does not offer a --generate-attachments may I let to you, maintainers, the work on the behat tests?
NB: rebased on master but 68ee7f7 is the same commit as before. Only 7bb16b2 is new.

drzraf · 2017-09-01T22:35:11Z

For the record I although wanted to include comments' images in my fixtures.
These images were the result of an ACF-field attached to comment (here inside the "illustration" ACF repeater field).

The method is thus to make a first dump (--with_comments)
Then running such an xpath expr:
xpath -q -e '//*[wp:meta_key/text()[starts-with(. , "illustration_repeater_")]]/wp:meta_value/text()'
... whose ID (ACF meta_value contains the attachment's post_id) could be added to a second wp export --post__in= run.

A long-term solution may be a hook allowing specific plugin to interpret the semantic of the meta_value for a specific meta_key (and, for example, add necessary post_id to the dump)

schlessera · 2017-09-24T11:21:33Z

src/WP_Export_Query.php

@@ -188,6 +194,11 @@ private function post_type_where() {
 			$this->wheres[] = 'p.post_type IS NULL';
 			return;
 		}
+
+		if ( $this->filters['with_attachments'] == FALSE && ( !$this->filters['post_type'] || !in_array( "attachment", $this->filters['post_type'] ) ) ) {


WPCS code style: space after !

in_array() should always be used with its third $strict argument set to true, to avoid getting unexpected results.

No need for double quotes for "attachment".

schlessera · 2017-09-24T11:26:32Z

src/WP_Export_Query.php

@@ -260,9 +271,9 @@ private function category_where() {
 		$this->wheres[] = $wpdb->prepare( 'tr.term_taxonomy_id = %d', $category->term_taxonomy_id );
 	}

-	private function attachments_for_specific_post_types( $post_ids ) {
+	private function include_attachments_ids( $post_ids ) {


Please rename to include_attachment_ids() (only IDs in plural).

schlessera · 2017-09-24T11:31:59Z

src/Export_Command.php

+		if ( is_null( $with ) )
+			return true;
+
+		if ( (int) $with <> 0 && (int) $with <> 1 ) {


--with-attachments should be handled as a flag, not a parameter taking a value. To improve the readability with flag negation ( --no-<flag> ), I'd like to also rename the flag to --include-attachments.

So, instead of having values like this:
--with-attachments=0 => off
--with-attachments=1 => on

...it should be the standard flag behaviour:
--include-attachments => on
--no-include-attachments => off

This will then also support setting a different default through a config file, and override it with --include-attachments instead.

yeah but currently, all other options use a "_" between words, so that would mean:
--no-with_attachments

Hmm, good point. I think the underscore in this command came about because it is using some of the variable names from WP.

I'll discuss this in tomorrow's office hours to see what the best approach is.

I discussed this with @danielbachhuber , and we agreed to use --with_attachments as flag, knowing that the negation will be a weird --no-with_attachments.

Do you need assistance with converting the parameter into flag notation?

…chments export, specifically in the case of --post__in Do not consider --post__in=0 as a valid value (ignore it smoothly) 0 is a special value for post without parent. When people use this value, for example --post__in=1,2,,3 --attachment=1 filter-out the spurious post__in=0 to avoid adding all attachment having post_parent = 0

drzraf · 2017-09-24T18:55:15Z

NB: I had no other option than fully recreate the branch because master changed too much since the original patch.

danielbachhuber · 2017-10-24T16:27:24Z

My biggest concern with this PR is that we aren't introducing a change in the existing behavior. It's not quite clear to me based on a cursory review, so would be worthwhile to make sure there are tests around the current behavior.

schlessera · 2017-10-24T16:28:15Z

I'll add tests by building the tests first, and only then applying the patch, just to stay on the safe side.

…rt files directly. As it turns out, `wp import` does not import attachments that don't come with an actual file to upload. Related wp-cli/wp-cli#928

schlessera · 2017-10-25T10:41:05Z

@danielbachhuber You can review and merge this PR now.

To make sure we don't change the default behaviour, I verified that the normal post__in export does not suddenly start adding attachments. This should be the only use case that could be impacted. That particular test succeeds both with or without this PR.

support exporting attachments when specific post ids are requested

vadimkantorov · 2024-11-08T11:09:11Z

Does --with_attachments have effect of actually embedding the attachments/image content inside the XML? (e.g. as CDATA/base64)

I'm interested in some portability between blogging platforms and the WXR/XML format could achieve this, if export/import can support embedding the images/attachments/resources inside the WXR.

swissspidy · 2024-11-08T12:44:34Z

I don't think WordPress supports base64/cdata in WXR files. Therefore it's not something WP-CLI supports.

I see you just opened #118 so let's continue discussion there instead of a 7 year old closed PR :)

schlessera added command:export enhancement labels Aug 30, 2017

danielbachhuber reviewed Aug 30, 2017

View reviewed changes

drzraf force-pushed the post_ids-attachments branch 2 times, most recently from 37c5135 to 7bb16b2 Compare September 1, 2017 21:42

danielbachhuber requested a review from schlessera September 13, 2017 06:47

schlessera requested changes Sep 24, 2017

View reviewed changes

drzraf closed this Sep 24, 2017

drzraf force-pushed the post_ids-attachments branch from ff9369c to b57e5f9 Compare September 24, 2017 18:49

drzraf reopened this Sep 24, 2017

danielbachhuber assigned schlessera Oct 24, 2017

schlessera added 6 commits October 25, 2017 10:12

Test whether attachment handling might change.

1c669c4

Test whether attachments are properly exported.

72c126d

Merge branch 'master' into pr/16

ab6ace4

Tweak test reading flow.

676ccec

Use flag getting helper for '--with_attachments'.

988e147

Rewrite tests to avoid wp import and check the contents of the expo…

96b942a

…rt files directly. As it turns out, `wp import` does not import attachments that don't come with an actual file to upload. Related wp-cli/wp-cli#928

schlessera approved these changes Oct 25, 2017

View reviewed changes

schlessera added this to the 1.0.4 milestone Oct 25, 2017

danielbachhuber self-requested a review October 25, 2017 12:22

danielbachhuber removed this from the 1.0.4 milestone Nov 21, 2017

settings bot removed the enhancement label Dec 6, 2017

Merge branch 'master' into post_ids-attachments

91bce7e

schlessera requested a review from a team December 29, 2017 11:38

Merge branch 'master' into post_ids-attachments

f01d860

gitlost approved these changes Dec 29, 2017

View reviewed changes

gitlost merged commit 4f7df86 into wp-cli:master Dec 29, 2017

gitlost added this to the 1.0.6 milestone Dec 29, 2017

drzraf mentioned this pull request Jul 12, 2018

can't set with_attachments to FALSE #40

Closed

danielbachhuber pushed a commit that referenced this pull request Nov 18, 2022

Merge pull request #16 from drzraf/post_ids-attachments

b3d6937

support exporting attachments when specific post ids are requested

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support exporting attachments when specific post ids are requested #16

support exporting attachments when specific post ids are requested #16

drzraf commented Aug 29, 2017

drzraf commented Aug 29, 2017

schlessera commented Aug 30, 2017

danielbachhuber commented Aug 30, 2017

danielbachhuber Aug 30, 2017

drzraf commented Aug 30, 2017 via email

drzraf commented Aug 31, 2017

danielbachhuber commented Aug 31, 2017

drzraf commented Sep 1, 2017 •

edited

Loading

drzraf commented Sep 1, 2017

schlessera Sep 24, 2017

schlessera Sep 24, 2017

schlessera Sep 24, 2017

schlessera Sep 24, 2017

schlessera Sep 24, 2017

drzraf Sep 24, 2017 •

edited

Loading

schlessera Sep 25, 2017

schlessera Sep 26, 2017

schlessera Sep 26, 2017

drzraf commented Sep 24, 2017

danielbachhuber commented Oct 24, 2017

schlessera commented Oct 24, 2017

schlessera commented Oct 25, 2017

vadimkantorov commented Nov 8, 2024

swissspidy commented Nov 8, 2024

support exporting attachments when specific post ids are requested #16

support exporting attachments when specific post ids are requested #16

Conversation

drzraf commented Aug 29, 2017

drzraf commented Aug 29, 2017

schlessera commented Aug 30, 2017

danielbachhuber commented Aug 30, 2017

Choose a reason for hiding this comment

drzraf commented Aug 30, 2017 via email

drzraf commented Aug 31, 2017

danielbachhuber commented Aug 31, 2017

drzraf commented Sep 1, 2017 • edited Loading

drzraf commented Sep 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drzraf Sep 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drzraf commented Sep 24, 2017

danielbachhuber commented Oct 24, 2017

schlessera commented Oct 24, 2017

schlessera commented Oct 25, 2017

vadimkantorov commented Nov 8, 2024

swissspidy commented Nov 8, 2024

drzraf commented Sep 1, 2017 •

edited

Loading

drzraf Sep 24, 2017 •

edited

Loading