hyyan / woo-poly-integration

Looking for maintainers! - Wordpress WooCommerce Polylang Integration
https://wordpress.org/plugins-wp/woo-poly-integration/
MIT License
183 stars 66 forks source link

Rewrite rules updating on every page call #248

Closed krzyc closed 6 years ago

krzyc commented 6 years ago

On every page load plugin executes two DB calls to refresh rewrite_rules: UPDATE wp_options SET option_value = '' WHERE option_name = 'rewrite_rules' UPDATE wp_options SET option_value = '[...]' which make excessive (and unnecessary in my opinion) database usage. Can you explain, whether this is expected behavior or a bug?

Jon007 commented 6 years ago

are you sure it is this plugin and can you provide any more details? What plugin versions are you using? Additionally, what Wordpress and Polylang settings are you using for Permalinks?

Jon007 commented 6 years ago

@hyyan - looks like confirmed bug:

WP_Rewrite flush_rewrite_rules() or flush_rules() does: update_option( 'rewrite_rules', '' ); and then resave them.

if you look at Polylang source code the rewrite flush is only called on saving options eg: add_action( 'update_option_page_on_front', 'flush_rewrite_rules' ); However in wooPoly this is being called on every init:

        add_action(
                'init', array($this, 'rewriteEndpoints'), 11
        );
hyyan commented 6 years ago

@jon007 Indeed it looks like a confirmed bug, I have written these part of code like more than two years ago, I believe this was the only possible way to get woocomerce account URLs translated from polylang strings table, but using update_option_page_on_front hooks will serve the same goal here

Jon007 commented 6 years ago

Reopening issue as commit not found in master branch

Jon007 commented 6 years ago

@hyyan I was trying this change but found it didn't work for the endpoints under My Account area: when not in primary language the links give 404 error, but as soon as the change is reversed, ie update_option_page_on_front is changed back to init, it works again.

        add_action(
                'update_option_page_on_front', array($this, 'rewriteEndpoints'), 11
        );

But it can't be necessary to save the rewrite rules again on every page load...!

Jon007 commented 6 years ago

by just commenting out the flush_rewrite_rules(); I discovered that the rules continue to work only for the last language served This explains why the rewrite is necessary.. because it is rules are only ever valid in one language, they must be saved again in case the new user is in a different language...

Jon007 commented 6 years ago

Well, the fix is this in the Endpoints.php, now the question is how to get this to be called reliably one time to save all the right rules..

    public function addEndpoints()
    {
        $langs = pll_languages_list();
        foreach ($this->endpoints as $endpoint) {
            foreach ($langs as $lang) {
                add_rewrite_endpoint(pll_translate_string( $endpoint, $lang ), EP_ROOT | EP_PAGES);
            }
        }
    }
philippehenri commented 6 years ago

Hi Jon

I replaced the function addEndpoints in endpoints. php and the infinite loop problem persist. It happen as soon as i activate the XML-RPC plugin (as described in my original posting #266 ) regards philippe

Jon007 commented 6 years ago

@philippehenri there are two changes in the commit,

  1. comment out the flush_rewrite_rules(); statement
  2. change the addEndpoints() to add endpoints for all languages

after applying you do need to save rewrite rules at least once for point 2 to take effect - for example by saving under settings, Permalinks.

Once the save is done, there is no need to continue saving it each time.

Note, this was originally raised as a perfomance issue, and I checked in query monitor, this was around the most expensive operation in my installation: it may be this is also a flaw in the XML-RPC plugin.

philippehenri commented 6 years ago

yep that seems to works :) thanks Jon

hyyan commented 6 years ago

@Jon007 This solution will never work, this is causing Orders under My account not to be translated, I will revert this until I find a better solution

Jon007 commented 6 years ago

@hyyan it works fine for me... on woo 3.1.2 did you save in Settings, Permalinks? is this another separate woo 3.2.x issue?

Actually maybe I didn't quite get the problem, let me retest some more.

Jon007 commented 6 years ago

@hyyan Retested... I have 1.0.3 plus this change in production with woo 3.1.2 and 3 languages

Next step 3.2.5 test..

hyyan commented 6 years ago

@Jon007 This is probably you flushed the URLs in the settings. Right ?!

Jon007 commented 6 years ago

Yes, you have do that, just once, since the previous version is only saving the permalinks in 1 language, that is why they are re-saving them on every call.

All is needed is to find the right hook to flush permalinks for first time activation of this plugin. ... I'm not sure how that part works, with upgrades.

hyyan commented 6 years ago

@Jon007 Got it, I want to avoid this behavior for now. Users will start complaining about this when they update. I would prefer to keep the current behavior until we find the right hook to do it.

Jon007 commented 6 years ago

Activation hooks are not fired for plugin updates: https://make.wordpress.org/core/2010/10/27/plugin-activation-hooks-no-longer-fire-for-updates/

so it would need a woopoly version number in the options table and check

if the saved woopoly version doesn't exist or it is less than 1.0.4 then flush the permalinks

hyyan commented 6 years ago

@Jon007 Hmmm, This is will work in fact , let me revisit the issue and try your suggestion this evening

Jon007 commented 6 years ago

ok sure... this is why I didn't close the issue.. the change is correct but it needs the first time activation sorting out before putting into a release.

hyyan commented 6 years ago

@Jon007 I've added an upgrade hook depending on this All we do for now in this hook is to flush the rules.

Jon007 commented 6 years ago

@hyyan it's not ideal to rely on that hook since people can update (or rollback) via FTP or file copy. I certainly do for components which are sensitive. Also not quite clear from the documentation but it may be that only runs from the admin update screen, so maybe not run if updating via wp-cli or from another plugin controlling auto-updates.

The best would be to have a woopoly-version option and check if that is not set or less than the current code version, then run the process.

hyyan commented 6 years ago

@Jon007 we save the plugin version in the DB now and compare