design of function qtranxf_excludeUntranslatedPostComments #17
Comments
herrvigg
added
enhancement
|
On second thought, if you really have a different solution, which is tested and ready to go, submit it with pull request, and we will go from there? |
|
What if we add these two lines at the beginning of the function: Did your error messages disappear? Is that good enough for you? If you need to still filter them, then try this: |
|
Please, also tell me if option "Hide Untranslated Content" makes sense to you at all? I personally do not use it, since I think people should decide themselves whether they want to read untranslated to their language post. They might choose to translate it themselves, but when it is hidden, then they do not know that it existed. |
|
Option "Hide Untranslated Content" perfectly makes sense. I run a couple of sites where i have some posts that are just not relevant for the other languages. plus: it's a seo issue - 'duplicate content'... regarding the 'people should decide themselves' thing: i run a different approach - people who are logged in can choose which languages they understand in the profile settings and see posts of all languages - the main lang (the one they choose in the browser / the url) always first. the "Hide Untranslated Content" option is especially effective for 'latest posts' widgets. if your last 3 posts are untranslated you could easily drive people away from your site showing foreign headlines in suggested posts, etc. |
|
Ok, point taken. Other people seem to also want it. What do we do about the code? |
|
i see a big drawback in the current approach of filtering for post_content: it just doesn't tell you anything about the actual language of a certain comment if the corresponding post is translated or not. i have multi-lingual posts, people tend to comment in the language they are reading. so the filter hides comments for untranslated posts but still shows comments in a different language. so it makes much more sense to add a lang meta to every comment when the comment is saved. the language would be the current user language. very easy. i'll provide code asap. |
|
We want to hide the comments for the untranslated posts, not the untranslated comments. We do not care about the language of comment, it can be anything, we just want consistency, if post is hidden, then all comments to it also hidden regardless to their language. If we also want to filter comments by specific language, then it would be a new and different option. Could you answer the question, if the code I cited makes your problem go away? Did you figure out under which condition a query without JOIN to posts table happens? I would much rather to do optimization later, because I plan to re-design the whole backend to be sane, as it is insanely inefficient now. I do not wish to do it in pieces, just for comments now. Please, answer the question if the code above makes your site work. Do you need filtering for the case when the problem happens. Would be also good if you tell me finally when and how it happens. |
|
i used the second snippet and it works. |
|
what i found out until now: the get_comments(); function by default doesn't JOIN posts. if you query comment meta it adds INNER JOIN wp_commentmeta and only if you query post related stuff (like post_status, post_type, etc.) it adds JOIN wp_posts. so a save join string would be you could test for both cases and only skip if the join string is more complex... |
|
checking the core for 'WP_Comment_Query' (https://core.trac.wordpress.org/browser/tags/4.1/src/wp-includes/comment.php) led me to another major flaw in the current filter. as you can see in line 430 a CACHED version of the query is returned - ignoring any altered clauses! therefore dynamically altering the clauses leads to wrong returns, i.e. wrong languages displayed. especially if users run object caches. i tested this in my 2015 setup. i had to clear the cache to see the correct comments. there are 2 safe possible solutions:
if it's even worse, the same problem applies to the posts! still checking... |
|
it's true. post queries get also cached. so the 'hide untranslated' option is badly broken because it shows the wrong languages... |
|
Why don't it get cached correctly at first time? |
|
I thought this cache is for one session only, different users do not affect each other, do they? |
|
You had to clear cache, because you were doing testing and changed the query in between the calls on the same session, is that correct? This should not happen at live site, will it? |
|
From the page http://codex.wordpress.org/Class_Reference/WP_Object_Cache:
|
|
Does it happen in your theme, that this query changes language during the same request? Which theme do you use, btw? If this is what you really need, then yes, we can set language in |
|
read the source: it generates a md5 cache_key from the DEFAULT $args. any altering of the clauses happens later and is ignored by the function because the cache key is the same not matter what you do with the clauses later. so it returns the new request from the cache assuming that queryvars not changed == output the same. with a caching plugin the cache is persistent. that means the next user gets YOUR output. wp generates cache for every different set of $args. so this clauses altering breaks queries with ANY theme. try it with 2015! (i did.) |
|
i wrote an alternative approach for posts filtering today. i set up an action to preg match the languages in use from the title of the current post in the loop and set a post meta IF the meta is not already there. (for backwards comp) then i alter the query with 'pre_get_posts'. |
|
I understand, but default arguments uniquely defines what is done later, except us adding a fork by language. If we add
Which one do you mean? Anyway, what I described with I am very glad you that you have better ideas and wish to implement them. I will be waiting for your pull request here. Meanwhile I will put in |
|
BTW, you never let me know which theme do you use? I am still curious. |
|
2.9.8.9 has your changes. Persistent cache did not work, because they cut all extra keys from |
|
so at least have a look at my working solution.. you get in lots of trouble if you break caching. people tend to not read any warnings and complain in the support forums later. believe me, i know exactly what i'm talking about... if you insist on using a hacky workaround for your 3.0 that you could use any query var from the default list (in the core code, linked above) - i guess a meta value would do the trick. as mentioned before, this would be very hacky for a major release... |
|
i have never worked with github - as soon as i find out how pulling a request works and where to do it, i'll send you my solution. |
|
about my theme: i use a selfmade framework with child themes for the sites under my direct control. other sites i admin use several themes... |
|
I did look into it right after the discovery, but there is no one we can change safely as far as I can tell. Which caching plugin do you use? I can see a lot of problems with persistent cache, which such plugin must deal with. What if one user submits new comment? It is not going to show up for anyone until cache is invalidated? It can get extremely complex. I would say that if caching plugin does not work in this case then it is a problem of caching plugin. Let them figure out a solution. Many other plugins can alter query result like we do, and I am sure they do not think about persistent cache either. Caching plugin must have its own ways around. Did you try the latest version? Does it work for you except persistent caching? Press Download button at plugin page: https://github.com/qTranslate-Team/qtranslate-x, which runs this link https://github.com/qTranslate-Team/qtranslate-x/archive/master.zip GitHub has become one of the most popular standard for doing shared development. You will have to get engaged with them in any case for many different reasons, if you are in this line of business. Fortunately, they have pretty good documentation. Shortly: you fork the repository, clone it to your local computer, modify the code, push modifications to your fork, then from your fork it is two clicks to submit pull request here. You will find documentation on each step. Please, try the latest version before anything else ;) |
|
add this to a meta query: |
|
i use w3tc, which is very common. afaik most caching plugins make the wp cache persitent, btw.
exactly. that's why the core invalidates the cache on any new comment. read the code.
definitely not. i'd say it's a flaw in the core because it doesn't check for altered clauses. the caching plugin has no chance at all to handle this because it simply builds up upon the core cache function.
wrong - i guess this approach is pretty specific: dynamically alter the SQL for not logged in users and for the exact same URL? i can't think of one plugin that alters the SQL to output different content at the same URL. |
|
I am sorry, @side777, about the delay in response. I got busy now with other things. Could you meanwhile explain the logic behind the following:
|
|
^^ this makes the query individual for every language and different cache data is saved. |
|
I understand that it makes key unique and helps caching, I am curious how does this affects the final query and why it is safe to implement. I can look it up, but I thought you would explained it to me since you already did the investigation. |
Thursday Feb 12, 2015 at 00:02 GMT
Originally opened as qTranslate-Team/qtranslate-x#17
This is a continuation of WP discussion https://wordpress.org/support/topic/the-latest-beta-from-github-is-good-for-testing-thank-you on the topic initiated by side777.
side777: I get a WordPress database error Unknown column 'wp_posts.post_content' in 'where clause' for query
SELECT * FROM wp_comments [...]but only on some sites of my multisite network. still investigating.side777: Unfortunately i could not reproduce the error with a clean 2015 - BUT get_comments() still returns an empty array! The 2015 recent comment widget DOES return the comments, but ALL, also for untranslated posts, though.
johnclause: I guess in some cases this query gets executed without wp_posts table included. What are those cases? Do we abort this function in such cases? Or do we force a reference to wp_posts in somehow?
side777: I think this is not a good approach anyway - in worst case you query 1000s of comments WITH the corresponding posts, PLUS filtering the post content - I wrote my own solution a while ago... I think it's way more effective to save the current language to a comment meta when the comment is saved. this way you can easily query comments by meta.
johnclause: Sure that would be much better. Unfortunately, qTranslate is not designed for big sites with many languages and huge content to begin with. It does have severe performance problems in many places, not just this one. My priority is to make it work in a minimal, but bug free way, so that people can continue to use it. Most of qTranslate* clients, I believe, are small bi-lingual (not multi-) sites and they are ok with this version of free translation framework. I would worry about the performance issues later, when time comes. For now we need to figure out how to fix this query issue as soon as possible, so that I could release 3.0. Then I will be happy to come back and redesign this piece as well as other places in a much more efficient way. And I believe that those missing
ifstatements, which we need to figure out now, will be useful to know when we do optimization.The text was updated successfully, but these errors were encountered: