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

Add version number constant #173

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

Conversation

jrfnl
Copy link
Member

@jrfnl jrfnl commented Oct 3, 2024

This PR proposes to introduce a version number constant for the plugin.

As things are, the WP Core tests on PHP 8.4 are failing on the Importer plugin - see PR #172. For now, the failing tests will be skipped on PHP 8.4 for WP Core, however, once the Importer plugin is fixed, these will be re-enabled.

In CI, this will work fine as the Docker container will automatically pull in the trunk version of the Importer plugin.

However, when contributors run the tests locally on PHP 8.4, they will need to make sure they have updated their local copy of the importer plugin in the tests/phpunit/data/plugins/ directory, as otherwise they will still see failing tests.

PR WordPress/wordpress-develop#7363 / commit https://core.trac.wordpress.org/changeset/59085 already improves the messaging for contributors about the need to have the importer plugin copied into their local repo clone when running the tests locally.

However, we cannot currently do a version check on the installed plugin to signal to contributors when they need to update their copy of the Importer plugin, which means that contributors can still end up with inexplicably failing tests due to their local copy of the Importer plugin being out of date.

A version constant, like proposed in this PR, is not an ideal solution, as it means that the Importer plugin will need to be loaded from the WP Core test bootstrap file to get the value of the constant and loading the wordpress-importer.php file will automatically also load the rest of the plugin files due to the hard require statements, even if the tests are run using a --filter and the Importer plugin is not needed for the tests which are being run. However, this is the best we can do for now without refactoring the complete plugin to a more modern setup without side-effects and using lazy loading of classes via an autoloader.

Now, I realize updating the version number in the constant on releases will be something easily forgotten. For that reason, I've chosen to not add this as a class constant to the WP_Import class, but as a global constant in the src/wordpress-importer.php as the version constant also needs to be updated in the file docblock of that file, so this should minimize the chance of the update of the constant being forgotten.

This PR proposes to introduce a version number constant for the plugin.

As things are, the WP Core tests on PHP 8.4 are failing on the Importer plugin - see PR 172.
For now, the failing tests will be skipped on PHP 8.4 for WP Core, however, once the Importer plugin is fixed, these will be re-enabled.

In CI, this will work fine as the Docker container will automatically pull in the `trunk` version of the Importer plugin.

However, when contributors run the tests locally on PHP 8.4, they will need to make sure they have updated their local copy of the importer plugin in the `tests/phpunit/data/plugins/` directory, as otherwise they will still see failing tests.

PR WordPress/wordpress-develop 7363 / commit https://core.trac.wordpress.org/changeset/59085 already improves the messaging for contributors about the need to have the importer plugin copied into their local repo clone when running the tests locally.

However, we cannot currently do a version check on the installed plugin to signal to contributors when they need to _update_ their copy of the Importer plugin, which means that contributors can still end up with inexplicably failing tests due to their local copy of the Importer plugin being out of date.

A version constant, like proposed in this PR, is not an ideal solution, as it means that the Importer plugin will need to be loaded from the WP Core test bootstrap file to get the value of the constant and loading the `wordpress-importer.php` file will automatically also load the rest of the plugin files due to the hard `require` statements, even if the tests are run using a `--filter` and the Importer plugin is not needed for the tests which are being run.
However, this is the best we can do for now without refactoring the complete plugin to a more modern setup without side-effects and using lazy loading of classes via an autoloader.

Now, I realize updating the version number in the constant on releases will be something easily forgotten. For that reason, I've chosen to not add this as a class constant to the `WP_Import` class, but as a global constant in the `src/wordpress-importer.php` as the version constant also needs to be updated in the file docblock of that file, so this should minimize the chance of the update of the constant being forgotten.
Copy link
Member

@dd32 dd32 left a comment

Choose a reason for hiding this comment

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

Personally; I'd not add a constant here and just use something like get_file_data() or some other core method to determine the version of the plugin installed.

But if you feel that a constant here is the best option, go for it.

@jrfnl
Copy link
Member Author

jrfnl commented Oct 4, 2024

Personally; I'd not add a constant here and just use something like get_file_data() or some other core method to determine the version of the plugin installed.

But if you feel that a constant here is the best option, go for it.

I'll happily consider alternative solutions. The problem with something like get_file_data() is that - as this is primarily intended for the tests and the test bootstrap at that -, the get_file_data() function may not have been loaded yet, though I suppose that's fixable via the position of the check in the bootstrap file.

Maybe get_plugin_data() would even be better ? I'm just trying to remember now whether that's only usable for installed/activated plugins or can be used on any file.

@dd32
Copy link
Member

dd32 commented Oct 4, 2024

I just figured if the plugin PHP is loaded to access the constant, so must the WordPress codebase, as loading https://github.com/WordPress/wordpress-importer/blob/master/src/wordpress-importer.php is likely to call functions that aren't otherwise defined (ie. add_action doesn't work, you use tests_add_action before bootstrap).

If you want to go hacky-hack on it though... $importer_version = trim( explode( ':', array_values( preg_grep( '/^[\s*]*Version:/i', file( 'path-to-importer' ) ) )[0] )[1] );.. I'll refuse to admit I ever suggested this chain of code ;)

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