Skip to content

Abstract meta CRUD into methods#174

Merged
schlessera merged 8 commits intowp-cli:masterfrom
spacedmonkey:functions-meta
May 15, 2018
Merged

Abstract meta CRUD into methods#174
schlessera merged 8 commits intowp-cli:masterfrom
spacedmonkey:functions-meta

Conversation

@spacedmonkey
Copy link
Copy Markdown
Contributor

@spacedmonkey spacedmonkey commented Apr 16, 2018

In this PR, I abstract off the meta crud into there own method. This means that can overridden from the parent class.

fixes #172
related: #158

schlessera
schlessera requested changes Apr 16, 2018
View reviewed changes
src/WP_CLI/CommandWithMeta.php Outdated
@@ -1,4 +1,4 @@
<?php
$this->update_metadata( <?php
Copy link
Copy Markdown
Member

@schlessera schlessera Apr 16, 2018

Choose a reason for hiding this comment

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

There's nonsense up here... ;)

src/Comment_Meta_Command.php Outdated
class Comment_Meta_Command extends \WP_CLI\CommandWithMeta {
protected $meta_type = 'comment';


Copy link
Copy Markdown
Member

@schlessera schlessera Apr 16, 2018

Choose a reason for hiding this comment

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

This should only have 1 blank line here.

src/Term_Meta_Command.php Outdated
protected function delete_metadata( $object_id, $meta_key, $meta_value = '', $delete_all = false ) {
return delete_term_meta( $object_id, $meta_key, $meta_value, $delete_all );
}

Copy link
Copy Markdown
Member

@schlessera schlessera Apr 16, 2018

Choose a reason for hiding this comment

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

Redundant blank line here.

@schlessera
Copy link
Copy Markdown
Member

schlessera commented Apr 16, 2018

Good approach, only a few style fixes and the removal of a random paste needed.

@schlessera schlessera added command:comment-meta-add Related to 'comment meta add' command command:comment-meta-delete Related to 'comment meta delete' command command:comment-meta-patch Related to 'comment meta patch' command command:comment-meta-update Related to 'comment meta update' command command:network-meta-add Related to 'network meta add' command command:network-meta-delete Related to 'network meta delete' command command:network-meta-patch Related to 'network meta patch' command command:network-meta-update Related to 'network meta update' command command:post-meta-add Related to 'post meta add' command command:post-meta-delete Related to 'post meta delete' command command:post-meta-patch Related to 'post meta patch' command command:post-meta-update Related to 'post meta update' command command:term-meta-add Related to 'term meta add' command command:term-meta-delete Related to 'term meta delete' command command:term-meta-patch Related to 'term meta patch' command command:term-meta-update Related to 'term meta update' command command:user-meta-add Related to 'user meta add' command command:user-meta-delete Related to 'user meta delete' command command:user-meta-patch Related to 'user meta patch' command command:user-meta-update Related to 'user meta update' command command:blog-meta-add labels Apr 16, 2018
@schlessera schlessera added this to the 1.3.0 milestone Apr 16, 2018
@spacedmonkey
Copy link
Copy Markdown
Contributor Author

spacedmonkey commented Apr 17, 2018

I have added some basic comments to this. @schlessera let me know if you want me to change them, I am not great for writing comments. Otherwise, I am happy with the PR as it is. Don't think there needs more work on it.

This PR needs to be merged before, #159 as that PR will need to changed, to implement changes in this PR.

The tests seem to be failing on trunk, but I think that is unreleated to my change...

@schlessera
Copy link
Copy Markdown
Member

schlessera commented Apr 17, 2018

Ah, it seems to break on trunk now because a default Privacy Policy page was added as part of the GDPR effort.

I'll fix the tests for that.

schlessera
schlessera requested changes Apr 17, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@schlessera schlessera left a comment

Choose a reason for hiding this comment

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

This now contains lots of files from your IDE, please remove them from the pull request.

I recommend adding the .idea folder to your global git ignore list.

@spacedmonkey
Copy link
Copy Markdown
Contributor Author

spacedmonkey commented Apr 17, 2018

My bad. That is a silly make. Sorry, added .idea to gitignore file.

@schlessera
Copy link
Copy Markdown
Member

schlessera commented Apr 17, 2018

@spacedmonkey I actually meant for you to add .idea to your own, global .gitignore file, not the project's one. The project's .gitignore should not need to care about the .idea folder, that is client-specific tooling.

See https://help.github.com/articles/ignoring-files/#create-a-global-gitignore

@spacedmonkey
Copy link
Copy Markdown
Contributor Author

spacedmonkey commented Apr 23, 2018

@schlessera Tests are passing and feedback done.

@schlessera
Copy link
Copy Markdown
Member

schlessera commented May 8, 2018

@spacedmonkey I pushed a change to add full docblocks to the methods.

While doing so, I noticed that the functions delete_comment_meta(), delete_post_meta(), delete_term_meta() and delete_user_meta() only accept 3 arguments, not the 4 that are currently being passed.

Currently, the WP_CLI\CommandWithMeta::delete_metadata() method has a fourth argument $delete_all = false which is accepted by the general delete_metadata() function, but not by the specialized versions.

Reading through the docs, it seems to me that this argument does not make any sense in our current context, and would be more useful for a separate command.

How do you suggest we proceed?

@spacedmonkey
Copy link
Copy Markdown
Contributor Author

spacedmonkey commented May 8, 2018

Thanks for the feedback @schlessera !

I have remove the delete_all flag in 3e480f4 . It doesn't map to the arguments of the object type functions. There is already functionality for deleting all meta of a object, by passing --all flag to delete meta command, so I don't see the value of having another delete all sub command.

@schlessera schlessera merged commit d44e0bf into wp-cli:master May 15, 2018
@spacedmonkey spacedmonkey mentioned this pull request May 17, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@schlessera schlessera schlessera approved these changes

Assignees

No one assigned

Labels

command:comment-meta-add Related to 'comment meta add' command command:comment-meta-delete Related to 'comment meta delete' command command:comment-meta-patch Related to 'comment meta patch' command command:comment-meta-update Related to 'comment meta update' command command:network-meta-add Related to 'network meta add' command command:network-meta-delete Related to 'network meta delete' command command:network-meta-patch Related to 'network meta patch' command command:network-meta-update Related to 'network meta update' command command:post-meta-add Related to 'post meta add' command command:post-meta-delete Related to 'post meta delete' command command:post-meta-patch Related to 'post meta patch' command command:post-meta-update Related to 'post meta update' command command:term-meta-add Related to 'term meta add' command command:term-meta-delete Related to 'term meta delete' command command:term-meta-patch Related to 'term meta patch' command command:term-meta-update Related to 'term meta update' command command:user-meta-add Related to 'user meta add' command command:user-meta-delete Related to 'user meta delete' command command:user-meta-patch Related to 'user meta patch' command command:user-meta-update Related to 'user meta update' command

Projects

None yet

Milestone

1.3.0

Development

Successfully merging this pull request may close these issues.

Making changes to meta objects leaves behind stale object caches

2 participants

@spacedmonkey @schlessera