Skip to content

Remove wp-content directory requirement#20

Merged
kopepasah merged 2 commits into
masterfrom
issue-18
Jul 4, 2017
Merged

Remove wp-content directory requirement#20
kopepasah merged 2 commits into
masterfrom
issue-18

Conversation

@lukecarbis

Copy link
Copy Markdown

Resolves #18

@coveralls

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 8aab747 on issue-18 into ** on master**.

3 similar comments
@coveralls

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 8aab747 on issue-18 into ** on master**.

@coveralls

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 8aab747 on issue-18 into ** on master**.

@coveralls

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling 8aab747 on issue-18 into ** on master**.

Comment thread php/class-plugin-base.php
$file_name = str_replace( \DIRECTORY_SEPARATOR, '/', $file_name ); // Windows compat.
}
$plugin_dir = preg_replace( '#(.*plugins[^/]*/[^/]+)(/.*)?#', '$1', $file_name, 1, $count );
if ( 0 === $count ) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lukecarbis Did you mean to remove this too?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I don't see the point of forcing the plugin to be inside a plugins or mu-plugins directory. Although I can't imagine a situation where having a plugin elsewhere would actually be the case, enforcing this is simply not required.

I've removed the preg_replace in c48cd3a in favour of double dirname. This makes the assumption that the plugin-base.php file will remain in a child directory of the main plugin. I think that it's okay to make this assumption for now, especially if we will be pursuing the ideas proposed in #12 later on.

@kopepasah

Copy link
Copy Markdown
Contributor

@lukecarbis I cannot think of a case where the content directory (WP_CONTENT_DIR) constant and the content URL (WP_CONTENT_URL) constant would be in a different location, so this seems okay to me. However, I am wondering why the use of content_url() over WP_CONTENT_DIR in this context.

@lukecarbis

Copy link
Copy Markdown
Author

@kopepasah The reason I didn't use WP_CONTENT_DIR is because I feel it would be best practice to include an if( defined( WP_CONTENT_DIR ) ) check first. And if it isn't defined, I'd likely fall back to content_url() anyway. Also, there's already precedent for using content_url() in this method.

@coveralls

coveralls commented Jun 29, 2017

Copy link
Copy Markdown

Coverage Status

Changes Unknown when pulling c48cd3a on issue-18 into ** on master**.

@westonruter

Copy link
Copy Markdown
Contributor

@lukecarbis

Copy link
Copy Markdown
Author

Happy to change it if there's a good reason to. Otherwise, any reason why we shouldn't merge?

@westonruter westonruter left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm cool with it if it checks out with @kopepasah.

@kopepasah kopepasah merged commit 192586c into master Jul 4, 2017
westonruter pushed a commit to xwp/wp-customize-snapshots that referenced this pull request Jul 16, 2017
@westonruter westonruter deleted the issue-18 branch July 16, 2017 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants