Skip to content
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

Bugfix: make extension_url contain correct url in all cases #3

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ottok
Copy link

@ottok ottok commented Dec 14, 2015

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.

@kprovance
Copy link
Member

The question becomes, what if one has their files in a place NOT wp-content? It's happened in the past. That also must be considered.

@ottok
Copy link
Author

ottok commented Dec 14, 2015

Why would the Redux code be outside of wp-content? It could be in a theme or plugin or must-use plugin, but in all cases in wp-content. Anyway, the old code was buggy, now it at least works for the WordPress plugin and theme use cases.

See also reduxframework/redux-framework#2754

@kprovance
Copy link
Member

People sometime setup their wordpress to put content out of wp-content. It happens. Look it up. :)

@ottok
Copy link
Author

ottok commented Dec 14, 2015

Seriously, you need to have more precise argumentation to make you pull request review comments actionable, references to "look it up" does not help when I just argumented that plugins and themes are always under wp-content.

Please be precise what do you want me to do next before you merge these changes?

@kprovance
Copy link
Member

I don't have to argue anything. However, I;m not in the mood to deal with more Redux users telling me how to run my project. @dovy said he would try it out in an environment that doesn't use wp-content. if it works, great. If not, overrules. It;s as simple as that. I'm sure he'll post back eventually. Cheers.

@ottok
Copy link
Author

ottok commented Jan 22, 2016

Hello!

I today updated my installation of Redux to 3.5.9 at our site (https://mariadb.org/) and it broken again due to the reasons this PR would have fixed. Any news if you are going to merge this?

@kprovance
Copy link
Member

I've already explained several times the flaw in this approach. I'm not getting into it again.

@reduxframework reduxframework locked and limited conversation to collaborators Jan 22, 2016
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.

For cases where wp-content is not found, fall back to old URL detection code.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants