Bugfix: make _extension_url contain correct url in all cases

[ottok]

The old code populated _extension_url with contents like
https://example.org/var/www/sites/htdocs/wp-content/plugins/redux-framework/
-> ReduxCore/inc/extensions/customizer/

New implementation is does not mind if Redux is embedded in a theme or plugin,
supports having WordPress installed in any directory and to avoid future bugs:
the new code is clean and easy to read.

[ottok]

Travis test failed due to timeout on Heroku. the new code is not flawed.

[ottok]

Test seems to fail due to missing Heroku credentials, not because an error in the code.

[dovy]

We perviously asked you to vet to make sure this didn't break anything. I can attempt to dig in, but only after you've made sure this works for all test cases. (Plugin, theme, symbolic links, etc). Otherwise, please use the extension filter like others do. :)

[ottok]

I am sure the new version I pushed today does not break anything and it fixes the bug I've found at two separate real sites. The new code includes my improvement that works for all scenarios where Redux framework is part of theme, plugin or mu-plugin and it falls back in other cases to old buggy way that apparently works better somewhere.

[kprovance]

Okay. Really, I am going to say this one more time. I don't know how to make you understand that hard coding 'wp-content' into your regex will NOT WORK ON EVERY POSSIBLE WP INSTALL! I have seen people rename their wp-content folder to something completely different, somehow thinking this will improve security. While the offered code will work in the typical install, it will fail where the WP_CONTENT_DIR and WP_CONTENT_URL has been changed. We typically do not look to break backward compatibility here, and personally, I don;t want to deal with the people who will complain we broke something, only to have to go back to the previous way we used. In other words, we just be causing issues where none need be.

[dovy]

@kprovance let's be nice...

@ottok what Kevin is saying is true. We've been down this road and spent months and months finding something that was close to correct. I know it's not perfect, but unless we have some solid checking from you we're not confident in changing it. We have over 800k users on Redux. This is a big change and we need it vetted big time.

If you want to put the time we're more than willing to hear your results. If you're not, then you have to wait for us to test it. But right now, it's not a huge priority because it's working for us and most users.

Have you tried using the filter?

The real problem is we can't always be sure Redux is a standalone plugin, embedded in a theme, or embedded in another plugin. It's just a pain. :(

[ottok]

That is why I made the fallback. Let's assume that indeed wp-content is not in the path. Then it is OK, it will not find it and fall back to the old detection code. The regex does not need to work on every possible install, thanks to the fallback which activates if no wp-content is found.

Currently I thought rewriting the URL relative to wp-content is an elegant solution and works for all the 99%, while the last 1% is server by the fallback I added today.

I am happy to write another version if you give me hints what alternative way there is to fix the bug with the current code. Some fix must be invented to fix current bug.

I don't know how many suffer from the current bug, but at least this one has the same symptoms: Redux Framework and the URL does not work as it has the file system path included instead of something that starts with /wp-contents/: https://wordpress.org/support/topic/theme-options-page-not-working

I also found #2197 - I guess that is the one you had bad experiences. In the commit f705552 the feature was broken by adding a long fixed string to the path which for (at least to be) obivious reasons will never work as the file can be located in any folder with any name.

[dovy]

Well then, I didn't see the fallback because I wanted more than "here's a fix". @kprovance let's give this a try. We can keep it in this repo and see if it works. :)

[ottok]

@dovy

I have not tried using a filter. I found the lines in the code that caused a bug so I decided the best approach is to fix the code at the root of the bug.

This solution does not rely on Redux being a standalone plugin. It relies only on the fact that a the single file where this piece of code is run resides anywhere in /wp-content/ be it a subdirectory of a subdirectory of a embedded plugin in a child them or whatever.

It is a fair answer that you want to do more tests and I should keep waiting. The only reason I started championing and argumenting more is that I initially got the reply that the solution has regressions.

So I will continue waiting until you have had the chance to verify this fix.

[dovy]

Than you for your attitude and wonderful response. I will take a look before the end of next week. Then I'd like to have it in the repo for a week or two before we push to WP.org to ensure we have all corner cases. I'm so glad you did a fallback. I was really scared of a strait-up replacement. :)

[ottok]

An additional tip: if you want to avoid regressions, try not make super complex oneliners like current $this->extension_url = site_url( str_replace( trailingslashit( str_replace( '\\', '/', ABSPATH ) ), '', $this->extension_dir ) ); - or if you do, please at least add comments to the code.

[dovy]

haha, i know. That's a windows ONLY bug. It took forever to solve too.

[kprovance]

Adding manually.

[dovy]

Thanks @ottok. We'll ask users to vet it. Can you provide us an example of where your code was failing so we can test in-house?