Add tested_up_to field
#413
Conversation
Can you elaborate on what adjustments you made? Does the class not support getting this information out of the box? I don't like maintaining our own fork of this class. I'd much rather pull in https://github.com/afragen/wordpress-plugin-readme-parser via Composer for example.
That makes sense to me. |
@swissspidy The class works fine but it is huge and parse part of the content that is not required in this case. So I took only the parts from the class that we need to get the headers, and adjust a few lines to adhere with the PHPCS in WP-CLI. Thanks for bringing up that library; I wasn't aware of its existence. I believe we can rely on it if it's the preferable approach. I'll test it out and make the necessary updates. |
src/Plugin_Command.php
Outdated
| 'wporg_status' => $wporg_info['status'], | ||
| 'wporg_last_updated' => $wporg_info['last_updated'], | ||
| ]; | ||
|
|
||
| if ( $this->check_headers['tested_up_to'] ) { | ||
| // Include information from the plugin readme.txt headers. | ||
| $plugin_readme = WP_PLUGIN_DIR . '/' . $name . '/readme.txt'; |
There was a problem hiding this comment.
Should we normalize file path here? Utils\normalize_path()? What do you think?
There was a problem hiding this comment.
May be we should handle readme.md also as it is supported in the directory.
There was a problem hiding this comment.
@ernilambar thanks for the feedback, and pointing out that function. Yes, I think we need to normalize the path. I will make the necessary update.
We could probably add support for the readme.md. But which file takes precedence assuming there are two readme files within the plugin? Is it the readme.md or readme.txt?
|
I noticed that the CI tests failed. The issue comes from the 🤔 There are a couple of things I could think of we could consider:
@swissspidy What's your thought? |
|
Well then I guess we have to fork it after all 🤷 Can be a file in this repo again. Does not need to be a separate package. |
|
I'd prefer not to fork something like this. Could we:
|
|
@danielbachhuber Andy is not maintaining it. The file is a 1:1 copy of what's on Meta with a regular sync. |
|
Honestly personally I don't see much value in showing the "Tested up to" information anyway |
|
@swissspidy as I dig in this issue further, I've just realized it would require extra maintenance work, especially since the parser isn't compatible with older PHP versions and doesn't adhere to the PHPCS rules in WP-CLI. It seems like a significant effort for a feature that may only provide little value. And as mentioned ☝️ doing this will adds some overhead, which could slow down performance in some scenarios. @danielbachhuber, at this point, I'm not sure if we'd still want to pursue completing this issue. |
|
@swissspidy 👋 I'd like to revisit this Pull Request since and push this forward if possible. One thing that makes this update a bit more complicated was due to the external dependency to parse the readme files. It is a direct copy of the one from WP.org, is limited to specific PHP versions, and there are a few files and dependencies there that simply copying would be inconvenient. So I decided to create a new package syntatis/wp-plugin-readme-parser. It is similar to Since it's Hack Day, I wanted to share it here to get some feedback and to see if using this package aligns with our approach for this issue. Thanks! |
|
@tfirdaus Do we need an entire dependency to pull the "Tested up to" value? I think I'd prefer some simple regex to avoid all of the complexities of a dependency. |
|
@danielbachhuber initial plan was actually quite simple. But thought on the discussions above ☝️ we prefer not maintaining this on our own and use an external library. Initial updates can be found 👉 12c5b34#diff-3df1a510df0bb92bdb36e3816eab6cb8d8a761db550713f56f19560762bf9a76 |
|
@tfirdaus Is there anything preventing the regex approach? |
|
@danielbachhuber nothing preventing using a simple regex. I only thought that if we could have close implementation as how WP.org parse the readme file. I could try it with the regex and see how it goes :) |
|
@danielbachhuber At the moment, this update only reads the |
|
@tfirdaus Let's just support |
|
@danielbachhuber sounds good 👍 |
This Pull Request is a PoC for addressing the issue reported in #241. While the changes made in this PR seems to be working, there are a couple of things I'd like to confirm:
tested_up_tooptional, similar to how we handlewporg_? This would mean that the file would only be parsed when the field is explicitly specified in the--fieldsparameter.