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

Use PATHINFO_BASENAME for interoperability #42

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

Conversation

widoz
Copy link
Member

@widoz widoz commented Mar 18, 2024

The usage of the flag PATHINFO_FILENAME might strip part of the name of the project.

One case might be a theme having a dot as part of the name, for instance when we want to differentiate the different versions of a theme we have in our installation.

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
The project base name is stripping part of the name when defining the baseName property

What is the new behavior (if this is a feature change)?
The part after the dot (.) character is no longer stripped off.

Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No

Other information:

The usage of the flag `PATHINFO_FILENAME` might strip part of the  name of the project. 

One case might be a theme having a dot as part of the name, for instance when we want to differentiate the different versions of a theme we have in our installation.

Signed-off-by: Guido Scialfa <[email protected]>
@widoz widoz requested review from gmazzap and Chrico March 18, 2024 17:56
@widoz
Copy link
Member Author

widoz commented Mar 18, 2024

I see some tests failing because of the changes, it might make sense to override the BaseProperties::sanitizeBaseName within the ThemeProperties instead?

@gmazzap
Copy link
Contributor

gmazzap commented Mar 18, 2024

@widoz I think we need to change a bit how it works.

The line substr_count($name, '/') and $name = dirname($name); in BaseProperties::sanitizeBaseName() is something that makes sense for plugins. In fact, for plugin we pass the result of plugin_basename() that with return the-folder/the-file.php when the plguin is in a folder, but the-file.php when the plugin is made of a single file.
So I think we should move the entire code of BaseProperties::sanitizeBaseName() to PluginProperties.

So, when we have $this->pluginBaseName = plugin_basename($pluginMainFile); we should do something alomng the lines of:

$basename = plugin_basename($pluginMainFile);
substr_count($basename, '/') and $basename = dirname($name);
$this->pluginBaseName = strtolower(pathinfo($name, PATHINFO_FILENAME));

This way, we should cover all cases for plugin, and it will also be backward compatible for plugins and libraries (which do not use files/directories at all, but Composer name).

At that point, BaseProperties::sanitizeBaseName() could probably removed at all, or maybe just strtolower($name).

  • For plugins and libraries it would already work as-is without the method, if we move current logic to PluginProperties
  • For themes, it mak sense that we use whatever value is retunred by get_stylesheet().

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.

2 participants