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

Not compatible with AnsPress Q&A plugin #10

Closed
dima-stefantsov opened this Issue Nov 18, 2015 · 19 comments

Comments

Projects
None yet
2 participants
@dima-stefantsov
Contributor

dima-stefantsov commented Nov 18, 2015

I am using AnsPress Q&A plugin and loving it very much.
I love AMT too, using it on all my sites. But this two plugins don't work well together.

I actually blame it on AnsPress implementation as I have read your code and it looks ok.

Nevertheless, you have said here https://anspress.io/questions/question/anspress-shortcode-in-some-meta/#li-comment-12519 that we should write a ticket if we want things fixed, and I do.

Take a look at my production site with AMT and AnsPress, question page I've made just for this issue http://dima.stefantsov.com/q/test-add-meta-tags-plugin/
Feel free to create any new questions or answers there if needed for your investigation.

As you can see, meta tags there are wrong. This is a serious SEO issue. Good thing I'm not having high hopes for that site. Yet :)

@dima-stefantsov

This comment has been minimized.

Contributor

dima-stefantsov commented Nov 18, 2015

The reason why I think it's not your fault, is because when I go to question page, and press "edit page" on the default wordpress admin bar, it takes me to editing of base page, where [anspress] shortcode is written.

If even wordpress doesn't know what REAL current page is, how could AMT know?

If my logic is right, it's AnsPress sources that should be fixed. I'm a wordpress noob. Do you know how is it usually done? Is there a good way to tell wordpress "here, this page - is not your base page with shortcode, but the real custom post type page"?

Questions are entities in "wp_posts" table, with unique urls and such. It should be somehow possible to tell wordpress what real document is.

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Nov 18, 2015

Hi Dima, I had found this particular question on your web site just before noticing this issue and added a comment to the question on the AnsPress web site. :)

I think AnsPress uses its own custom implementation and does not use the WordPress post objects, otherwise AMT would return the proper information since it uses the WordPress API to retrieve it from the post objects.

It is possible to filter the metadata AMT generates and fix the relevant information (after retrieving the correct values using the AnsPress API), but that would require some manual work and some custom extra code.

If AnsPress uses its own implementation, I'm very hesitant to add support for it in add-meta-tags.

I'll take a closer look at this issue in the following days and see what can be done.

Thanks for your detailed feedback and also for setting up a question for testing. It is much appreciated!

Kind Regards,
George

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Nov 19, 2015

Hi Dima,

Anspress is an interesting plugin, but the way it has been implemented requires that AMT adds internal support for Anspress' specific custom implementation before AMT can generate the correct metadata.

Unfortunately, this is not the way I do things with Add-Meta-Tags as I prefer to stick to the documented WordPress API for everything. This ensures compatibility with other plugins and, also, interoperability with any other plugins that utilize the WordPress API in order to make their functionality available to the web site. For instance, if Anspress used the WordPress API in its implementation, then Add-Meta-Tags would work out of the box.

Moreover, the fact that the developer's responses in the relevant questions are basically off-topic prompts to use a very specific 3rd party WordPress plugin, without providing any helpful hints at all about how the issue could be worked around, and also the lack of a documented public API tell me to stay away from this, at least for now.

So, I could help you with the basics about how to filter and fix the AMT metadata, but, unfortunately, at this time I don't have the time that is required in order to dive into the internals of this plugin and find ways to retrieve the correct meta information and offer a complete workaround for this problem. I'm really sorry about that, but this is the best I can do, at least until there is a public documented API about how this info could be retrieved.

George

@dima-stefantsov

This comment has been minimized.

Contributor

dima-stefantsov commented Nov 19, 2015

Hi George!

I'm not even thinking of building this support into AMT, more like fixing AnsPress itselt. I've committed to AnsPress github few times, Rahul (author) is prompt with merging pull requests, so I think I / we will be able to fix this issue no problem as well.

As far as I remember, this commit fixed the "wrong title displayed in frontend" issue we was having: anspress/anspress@35fd910#diff-587d8d534b87737cf5b3b26349c752d2L216

If you'd show me what exactly have to be set for AMT to be able to read it, I should have no much problem committing it to AnsPress.
This one will help: "I could help you with the basics about how to filter and fix the AMT metadata"

On a sidenote, I still have a plans for reading into AnsPress sources and finding out, will it be easy to change current behaviour to "default wordpress". In theory it should be. Questions are posts of custom type there, I don't see why they can't be used directly.

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Nov 19, 2015

Dima, that's absolutely great to know and actually means that the resolution of any interoperability issues with other plugins is just a matter of time.

Until this happens, I'll be glad to help with any customization effort, but as far as I can tell that will require generating all metadata (basic, opengraph, twitter cards, schema.org microdata, schema.org json+ld, and possibly dublin core) from scratch, which is actually more work than it initially seems, and inserting it into add-meta-tags.

BTW, the next AMT release will have some extra filter hooks which are going to be used to add internal support for bbPress and BuddyPress in future releases. Those hooks could possibly be used in order to attach the customization functions that generate the AnsPress metadata.

Please, feel free to ask your questions. For the time being, the issue will remain open. The wontfix label is there to remind me that there is nothing that should be implemented/fixed in AMT. I'll post some general sample code that utilises the new filter hooks as soon as 2.9.7 is rolled out.

George

@gnotaras gnotaras removed the wontfix label Nov 19, 2015

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Nov 19, 2015

I'm not even thinking of building this support into AMT

Actually, I'm open with adding support for popular 3rd party plugins, like it happens with WooCommerce, Easy Digital Downloads (EDD) (bbPress and BuddyPress are planned). But these plugins use regular post objects, so not all has to be generated from scratch and they also have a well documented API which can be used to retrieve their content type specific information.

Maybe it's a little too early for AnsPress. But, the dev seems devoted to the project, so it's only a matter of time.

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Nov 28, 2015

Hi Dima,

I won't be very active with development during the following weeks due to lack of free time. Nevertheless, I'll keep an eye on all the open issues about this topic, and time permitting I'll try to explore again all possible approaches for a solution.

Best Regards,
George

@dima-stefantsov

This comment has been minimized.

Contributor

dima-stefantsov commented Dec 4, 2015

Pull request #13 needed work. Look at #14, it's good.

@dima-stefantsov

This comment has been minimized.

Contributor

dima-stefantsov commented Dec 4, 2015

Here is what AnsPress developer answers about it's non-default routing https://anspress.io/questions/question/anspress-routing/

There obviously could be different good solutions, but I currently see an easy one, and I don't want to look further.

I have created a pull request that would solve this issue.
I am already running this AMT code on my live site, with following snippet:

add_filter('amt_get_queried_object', 'd_anspress_advancedmetatags_fix_post');
function d_anspress_advancedmetatags_fix_post($post)
{
    // Only make changes when inside AnsPress.
    if (!function_exists('is_anspress') || !is_anspress()) {
        return $post;
    }


    // Feed AMT with real question-post.
    if (is_question()) {
        return get_post( get_question_id(), OBJECT );
    }

    return $post;
}


add_filter('amt_get_options', 'd_anspress_advancedmetatags_fix_options');
function d_anspress_advancedmetatags_fix_options($options)
{
    // Only make changes when inside AnsPress.
    if (!function_exists('is_anspress') || !is_anspress()) {
        return $options;
    }


    // Never generate schema.org for AnsPress.
    $options['auto_schemaorg'] = '0';

    return $options;
}

Now it finally works together!

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 4, 2015

Hi Dima,

Thanks a lot for your work! It is much appreciated.

BTW, I just noticed that the bitbucket repository (v2.9.9) had not been synced with this one on Github. I'm really sorry about that. I'll have to find a robust automatic way to do this syncing every time I push changes to bitbucket.

I'll need some time to review the pull request and the above snippet and check this approach.

I assume that the issue with repository syncing has caused some problems (the PR involves changes to .po, .mo files etc.). Once I have checked it more closely, would you mind making changes or create a new PR, if that is needed?

Best Regards,
George

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 4, 2015

Hi Dima,

A filter hook that can be used for the exclusion of specific types of metadata through filtering is planned, so managing this through a amt_get_options hook does not seem like the optimal way to go. I'll soon push a commit that incorporates this exclusion hook.

Still investigating about amt_get_queried_object... Possibly the post object could be filtered using one of the built-in hooks, but I'll need to look into it some more..

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 4, 2015

A filter hook that can be used for the exclusion of specific types of metadata through filtering is planned, so managing this through a amt_get_options hook does not seem like the optimal way to go. I'll soon push a commit that incorporates this exclusion hook.

This metadata exclusion hook has been implemented in this changeset

Excluding schema.org metadata for anspress:

add_filter('amt_exclude_schemaorg_metadata', 'd_anspress_exclude_schemaorg');
function d_anspress_exclude_schemaorg($default) {
    if (!function_exists('is_anspress') || !is_anspress()) {
        return false;
    }
    return true;
}
@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 4, 2015

BTW, still researching the optimal way to filter the post object as returned by get_queried_object(). If there is no suitable built-in hook for this purpose, adding a filter hook as implemented in the PR is the way to go.

@dima-stefantsov

This comment has been minimized.

Contributor

dima-stefantsov commented Dec 4, 2015

Hi George, thanks for your efforts.
At first I thought your implementation is too restrictive, because if I had to change many options (I currently disable schemaorg and dublin core for AnsPress) I'd have to write a lot of filters. But then I wrote this:

// Not tested but should work.
add_action('wp', 'd_anspress_amt_fix');
function d_anspress_amt_fix()
{
    // Only make changes when inside AnsPress.
    if (!function_exists('is_anspress') || !is_anspress()) {
        return;
    }


    add_filter('amt_exclude_schemaorg_metadata', '__return_true');
    add_filter('amt_exclude_dublin_core_metadata', '__return_true');

    // Feed AMT with real question-post.
    if (is_question()) {
        add_filter('amt_get_queried_object', function(){ return get_post(get_question_id(), OBJECT); });
    }
}

Now it's just a single action to set up it all. Good job!

I'm still curious, why you think filtering options itself is not a good idea? (I guess because it's called many times in a row in your code)

optimal way to filter the post object as returned by get_queried_object(). If there is no suitable built-in hook for this purpose

I've tried to look for it in sources before implementing it in a way of PR, looks like they just get it and return it, no filters.

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 5, 2015

Hi Dima,

I'm still curious, why you think filtering options itself is not a good idea? (I guess because it's called many times in a row in your code)

Yeap, I'm trying to keep the extra calls to apply_filters() at a minimum (there are too many already) and also the fact that it is a little different than the way other things can be customized. However, this is very subjective and has to do with me and the way I do things. Your solution was perfectly fine technically.

I've tried to look for it in sources before implementing it in a way of PR, looks like they just get it and return it, no filters.

I've incorporated your amt_get_queried_object filter hook as it was the only way to do it. I also took a lot at the WP sources and surprisingly enough didn't find any other way to do it.

BTW, I've used the name and email address I found in the patch of the PR, so that the changes are attributed to you.

However, there is a small addition. The filtering function that is attached to the amt_get_queried_object hook accepts two arguments: the post object and the plugin options.

function my_filtering_function($post, $options) {
    // ...
    return $post;
}
add_filter('amt_get_queried_object', 'my_filtering_function', 10, 2);

Your suggestions are welcome.

Thank you for contributing!

George

@gnotaras gnotaras added enhancement and removed needs-feedback labels Dec 5, 2015

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 5, 2015

Also, I'd greatly appreciate if you tested it and provided some feedback about whether it works fine or not.

I'll keep this issue open for a while, just in case something else needs fixing.

@dima-stefantsov

This comment has been minimized.

Contributor

dima-stefantsov commented Dec 5, 2015

I was unable to follow RELEASE_PROCEDURE.txt instructions, couldn't python make_release.py (tried installing both python 3.5 and 2.7).

Copied files manually into my production site, changed snippet to the one I wrote above, I think it works fine (even though it was written for the old filter signture with just one parameter).

Thanks.

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 6, 2015

I was unable to follow RELEASE_PROCEDURE.txt instructions, couldn't python make_release.py (tried installing both python 3.5 and 2.7).

These python scripts have some external dependencies and are not really meant for general use. But, now that you've mentioned it, maybe I'll also include a script that sets up a virtual python environment for running these scripts effortlessly.

Thanks for your feedback! I'll make a new release in the following days.

George

@gnotaras

This comment has been minimized.

Owner

gnotaras commented Dec 10, 2015

Dima, I've released 2.9.10 with the aforementioned changes. I'll now close this issue. Nevertheless, please feel free to send extra feedback.

Thank you.

George

@gnotaras gnotaras closed this Dec 10, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment