search

wp-papi / papi

:rocket: WordPress Page Type API with custom fields
https://wp-papi.github.io
MIT License
263 stars 32 forks source link

[Repeater] - Polylang compatibility / meta saving bug #222

Closed nlemoine closed 5 years ago

nlemoine commented 7 years ago

I recently worked on a multilingual project using Polylang and stumbled upon a weird issue with the repeater property.

What happened

Issue description

From what I quickly figured out, here's some details of this issue.

I didn't dig too deep in Polylang internals but the big picture is that it seems to deal with translations by creating a language taxonomy.

Therefore, when you add/edit a post, Polylang also adds/edits some terms which triggers some actions in papi that have the same handlers.

In the end, these handlers run the remove_repeater_rows method in repeater property which receive a term ID. If you're lucky, the term ID doesn't match a post ID containing repeater metas. But sometimes, when the term ID matches a post ID containing repeater property, all the repeater data is deleted.

Steps to reproduce

This may be tricky to reproduce, I can provide you a dump with "working" bug if you want.

What versions of softwares are you using?

frozzare commented 7 years ago

About compatibility

First off all, Papi don't have any compatibility support for compatibility or some other plugin, if you have compatibility support for one plugin you will end up with all so that's why Papi don't have any compatibility support.

If it's a problem where Polylang and Papi can't work side by side we may have to add some filters so we can create a polylang.php file at the compatibility repo. That's the best way to solve compatibility support between Papi and another plugin.

The issue

It sounds like Polylang runs terms hooks and Papi can't find the right meta type, that would be the only way that a term ID can be a post ID.

This issue will be tricky to reproduce as you say and since I don't use Polylang it will be any trickier. If you could provide a dump it maybe easier to produce, but it would be great if you could dig into this issue, I don't have so much time to work on Papi anymore and I'm focusing on to get 3.2 ready as soon as possible.

nlemoine commented 7 years ago

Compatibility

The issue raised with Polylang but I think it could happen in any context where term creation/edition would be triggered. I guess it's a bit more global issue.

Issue

I'll try to dig further on this issue and will let you know when I figure out what's going on.

frozzare commented 7 years ago

Compatibility

Have never experienced this problem before so I'm not sure about that this is a global issue. Language plugins does strange things.

Issue

Great! It could maybe be that easy that we check if current_filter is a term hook or not to return the right meta type.

nlemoine commented 7 years ago

Compatibility

Language plugins does strange things.

They certainly do. But I'm pretty sure actions like edit_term or created_term can easily occur when adding post tags or categories to a post.

Issue

I had a quick look on this issue today but might need your help as papi saving logic is not so easy to follow. You seem to have recently refactored some part of the saving api. I still need to get familiar with it and I'm lacking time right now.

Here's some things I noticed meanwhile:

I created a fork of Bedrock to make the testing as easier as possible. You can clone the project here: https://github.com/nlemoine/bedrock/tree/papi-fix-meta-saving I emailed you a download link with a database dump to test the issue (have a look in the "screen" post type). The post titles should guide you in reproducing the bug, please let me if you need anything else.

I'll work on this later in the week.

frozzare commented 7 years ago

Will "[TEST] – Remove repeater metas from 15" and look" delete repeater rows from post or term meta with id 15? Both the meta data on post and term 15 are still left when I update "[TEST] – Remove repeater metas from 15".

You can be right about that other plugins triggering created_term and edit_term may cause problem but only if it provide the nonce that papi checks after and that shouldn't happen. It's hard to protected the plugin from this kind of behavior. The only thing I can think of is to delete the nonce from $_POST data array and hope that it will solve the problem.

I haven't seen this problem before and I'm using dev-master version on a new site with both flexibles and repeaters without any data saved to wrong table, but we don't have any other plugin that does database saves on term hooks, just only a plugin that send data from wp to a http url on term hooks.

nlemoine commented 7 years ago

Will "[TEST] – Remove repeater metas from 15" and look" delete repeater rows from post or term meta with id 15? Both the meta data on post and term 15 are still left when I update "[TEST] – Remove repeater metas from 15".

The only thing I can think of is to delete the nonce from $_POST data array and hope that it will solve the problem.

I don't think nonce will solve this issue. I might have found a solution.

I think there are two main issues here:

So we have to filter before these actions are binded and only bind them when the type matches. I tried a quick and dirty fix that solves the issue: https://github.com/nlemoine/papi/tree/issue-222 It works for me but I can't tell if there are any unwanted side effects. Let me know what you think about it.

Do tests cover this part? I didn't run them.

frozzare commented 7 years ago

the meta type guessing is happening too late in the saving process. For now, all saving actions (save_post, created_term, edit_term) for the meta handler are triggered without any context.

Yes, you right here. But you should add a add_action( 'admin_init', [$this, 'admin_init'] ) and then have the code in the method, doing anonymous functions in a class as you do isn't okey, I only try to use it when not in a class.

What it would look like:

protected function setup_actions() {
  add_action( 'admin_init', [$this, 'admin_init'] );
  ...
}

public function admin_init() {
  switch ( $this->get_meta_type() ) {
    case 'post':
      add_action( 'save_post', [$this, 'save_meta_boxes'], 1, 2 );
      break;
    case 'term':
      ...
   }
}

some part of the meta type guessing in the Papi_Admin_Meta_Handler relies on a wrong parameter. The current_filter() does not guaranty that the saving process is happening on the matched type (which this issue is actually all about). For example, wp_insert_post (that would trigger save_post) could happen on an edit term page or wp_insert_term (that would trigger created_term) could occur on an edit post page (just like here).

I'm know that the current_filter don't guaranty saving process and that isn't what get_meta_type does, get_meta_type only try to find out if it's a post or a term and nothing else. So the way it uses current_filter is not wrong, the problem is more the first point, but if it solves to remove current_filter to get the right meta type and nothing else breaks I'm fine with removing it.

But papi_get_meta_type will not guaranty that it's saving process, it does the same as current_filter but in a more complex way to find out if it's a post, term or option. So it may not solve the issue 100%.

I think the best would be to check if papi_get_page_type_key exists value exists in the $_POST array and then add the hooks, but it may not solve the issue to 100% either. Maybe a solution where we add both meta type check and page type key check?

When you do a pull request you should always run the tests to see that you don't break anything even if the code you write don't have any tests (it's hard to get a full test coverage for wp plugins) and if you can you should try to add tests if no tests exists and be sure to check so wpcs don't return any errors (make lint:php)

nlemoine commented 7 years ago

But you should add a add_action( 'admin_init', [$this, 'admin_init'] ) and then have the code in the method, doing anonymous functions in a class as you do isn't okey, I only try to use it when not in a class.

Sorry about that, that's why I mentioned that it was a quick and dirty fix. The purpose was to get your thoughts on the solution itself. Of course I would implement it in a better way if I had to make a PR ;)

I think the best would be to check if papi_get_page_type_key exists value exists in the $_POST array and then add the hooks, but it may not solve the issue to 100% either. Maybe a solution where we add both meta type check and page type key check?

I may miss something but I don't get how page type key can provide a hint on the meta type to save (term or post here, options are handled in a different class)? I think post and term pages have pretty unique hidden fields that could be reliable to check against. Something like isset($_POST['post_ID']) for posts and isset($_POST['taxonomy']) for terms should ensure that we're on the right page. What do you think?

be sure to check so wpcs don't return any errors (make lint:php)

I ran this command but it returns multiple errors, not only on the files I edited. Code standards don't seem to be checked in travis. Am I missing something?

I'm currently installing the deps needed to run the tests.

frozzare commented 7 years ago

I may miss something but I don't get how page type key can provide a hint on the meta type to save (term or post here, options are handled in a different class)? I think post and term pages have pretty unique hidden fields that could be reliable to check against. Something like isset($_POST['post_ID']) for posts and isset($_POST['taxonomy']) for terms should ensure that we're on the right page. What do you think?

Yeah maybe you right, I have to think about this for a while before I have a good answer and will check with my colleague @rasmusbe what he think of this issue too. But you're solution isn't that bad and I think I have some similar when I want to find the attachment post id (since posts and attachment don't have the same data structure).

I ran this command but it returns multiple errors, not only on the files I edited. Code standards don't seem to be checked in travis. Am I missing something?

No, the link don't exists on travis, should maybe add it there and why you get a lot of errors is because a new version of wpcs this week and I haven't had time to update the projects phpcs.xml. Will fix as soon as possible and add travis checks as well.

If you need a test environment and know docker you can use this container https://github.com/frozzare/docker-wptest

I use myself and think it's good solution when you don't have VVV or something like that. (I think some utf8 tests (slug tests I think) or something failes with docker-wptest, will fix them as soon as possible too)

frozzare commented 7 years ago

Maybe a solution like this would work, check if save_post is fired with did_action and check if current_filter is term, if yes, don't continue and the same for term.

public function save_meta_boxes( $id, $post = null ) {
...
    // Bail if current filter is a save post filter and created or edit term has fired.
    if ( ( did_action( 'created_term' ) || did_action( 'edit_term' ) ) && current_filter() === 'save_post' ) {
        return;
    }

    // Bail if current filter is a save term filter and save post has fired.
    if ( did_action( 'save_post' ) && in_array( current_filter(), ['created_term', 'edit_term'] ) ) {
        return;
    }
...

I have added lint check to travis and fixed issue with lint and new rules.

frozzare commented 7 years ago

@nlemoine ping

nlemoine commented 7 years ago

Sorry @frozzare, I forgot about this and have been busy last weeks. I'll look at this within the week.

frozzare commented 7 years ago

@nlemoine ping :)

nlemoine commented 7 years ago

Sorry for the late answer @frozzare, too many projects to finish before going on holidays.

Maybe a solution like this would work, check if save_post is fired with did_action and check if current_filter is term, if yes, don't continue and the same for term.

The purpose here is to find out if which type (post, term, option, etc.) we're saving and I'm not sure your solution is reliable enough.

First, did_action is wrongly named, it actually gets the number of times an action has been fired. So did_action( 'created_term' ) does not mean the action won't happen later.

I think the code will even prevent some post meta from being saved because in a regular use, edit_term or created_term will very likely be fired (with methods like wp_set_post_terms in wp_insert_post).

Checking on $_REQUEST for some keys seems a better solution to me. I'll have a look at the saving process on other custom fields library to see there's something better.

nlemoine commented 7 years ago

ping @frozzare, what do you think?

frozzare commented 7 years ago

Sorry, forget about this.

  1. To find out which meta type is used you can use papi_get_meta_type function.
  2. But with did_action you can tell if a action has happen or not, not interested in the number, just knowing if it has happen or not.
  3. Yeah you maybe right since you can create terms from post page.
  4. That would be a good solution and would happy to accept a pull request. Note that the papi project isn't maintained anymore since I don't have the right focus to take Papi to the next level, you can read more about this in the readme. Each pull request and issues that are created from now will get a automatic reply about this, just to letting the users know about my decision about Papi.