Automattic / newspack-popups

Popup notifications.
GNU General Public License v2.0
70 stars 17 forks source link

wp-content directory is hard coded, not working with custom directories #612

Closed yogeshbeniwal closed 2 years ago

yogeshbeniwal commented 3 years ago

wp-content directory is hard code at couple of places, this leads to missing config file while using custom wp-content directory

https://github.com/Automattic/newspack-popups/blob/91b056bfa7ea51ffffe9bd7312a395edb8de5be5/api/setup.php#L9

https://github.com/Automattic/newspack-popups/blob/91b056bfa7ea51ffffe9bd7312a395edb8de5be5/api/setup.php#L26

aschweigert commented 2 years ago

@yogeshbeniwal do you have an example of where else the config file might live? We could make this a little safer but might also be able to just cover it with clearer setup docs...

yogeshbeniwal commented 2 years ago

@aschweigert Can use WordPress constants to access file path.

So far only these 2 lines are creating issue, but can see wp-content/ hard coded in more files with search in newspack-*

adekbadek commented 2 years ago

Hi @yogeshbeniwal , thank you for flagging this. To understand it better – the problems arise when the wp-content directory is named differently?

yogeshbeniwal commented 2 years ago

@adekbadek Yeah

adekbadek commented 2 years ago

Why would it be named differently – what's a use case of renaming it?

yogeshbeniwal commented 2 years ago

@adekbadek Administrators do it to hide WordPress (One can argue about it). Projects like Bedrock has custom name by default.

aschweigert commented 2 years ago

This feels like a bit of an edge case but it's a quick fix so it's doesn't really hurt to get it in the next batch of bugfixes.

@yogeshbeniwal you mention it showing up elsewhere in this repo but I'm not seeing it anywhere other than the file you pointed out, in the readme and in the related tests...is there anywhere else you're seeing it that I'm missing?

yogeshbeniwal commented 2 years ago

@aschweigert Here you go, it's in main newspack plugin.

/newspack-plugin/vendor/composer/installers/src/Composer/Installers/WordPressInstaller.php: 'plugin' => 'wp-content/plugins/{$name}/', /newspack-plugin/vendor/composer/installers/src/Composer/Installers/WordPressInstaller.php: 'theme' => 'wp-content/themes/{$name}/', /newspack-plugin/vendor/composer/installers/src/Composer/Installers/WordPressInstaller.php: 'muplugin' => 'wp-content/mu-plugins/{$name}/', /newspack-plugin/vendor/composer/installers/src/Composer/Installers/WordPressInstaller.php: 'dropin' => 'wp-content/{$name}/',

adekbadek commented 2 years ago

This feels like a bit of an edge case but it's a quick fix

Not exactly, unfortunately. The API of this plugin is outside of WordPress (for performance reasons). It makes the assumption that the WP_CONTENT_DIR is …/wp-content - this is hardcoded:

https://github.com/Automattic/newspack-popups/blob/bb5b1cb871ca8b7f9f4c99f918c6892b49658f3b/api/setup.php#L18-L20

aschweigert commented 2 years ago

@adekbadek Ah, I see, so we can't use content_url() there without loading WP? Do you have any other ideas for how to make this configurable? or should we maybe just cover it in the setup docs?

aschweigert commented 2 years ago

@yogeshbeniwal Ah, ok. Thanks. Let's open a new issue there and link to this one once we decide how to proceed.

adekbadek commented 2 years ago

Ah, I see, so we can't use content_url() there without loading WP?

Correct. And content_url() just reads WP_CONTENT_URL.

Do you have any other ideas for how to make this configurable?

The hardcoded instances could be changed to WP_CONTENT_DIR, which maybe could be defined before the API code is executed – but this would have to happen outside of WP anyway.

yogeshbeniwal commented 2 years ago

@adekbadek There is another line in which wp-content/plugin path is hard coded. Will leave that to your's awesome coding skills.

https://github.com/Automattic/newspack-popups/blob/bb5b1cb871ca8b7f9f4c99f918c6892b49658f3b/api/setup.php#L9

matticbot commented 2 years ago

:tada: This issue has been resolved in version 1.45.0-alpha.1 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket:

matticbot commented 2 years ago

:tada: This issue has been resolved in version 1.45.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: