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

Can I use "readme.txt" in the "View details" info? #217

Closed
efc opened this issue Apr 10, 2015 · 44 comments
Closed

Can I use "readme.txt" in the "View details" info? #217

efc opened this issue Apr 10, 2015 · 44 comments
Assignees

Comments

@efc
Copy link

efc commented Apr 10, 2015

Plugins installed with GitHub Updater seem to use only the description and changelog markdown when displaying their "View details" info. However, I have a full "readme.txt" file with a number of sections that should create a much more informative "View details" box.

Is there any way to use this readme.txt if it is present?

@efc
Copy link
Author

efc commented Apr 10, 2015

I just noticed in another issue that you may intentionally be avoiding "readme.txt" because you don't think it occurs often enough and it would require another API call.

Another option that I would be OK with would be to extend the README.md parsing to make sections (each first level headline) into tabs in the "View details" so that you could continue to use the markdown parser. I just hope to be able to provide richer details than the one line description.

@efc
Copy link
Author

efc commented Apr 12, 2015

I found a way to do this without interfering with the current behavior too much. Please take a look at the parsing-readme branch I just published.

This branch has code that only jumps into action if there is no changelog file in the plugin repo. In that case, it assumes the author of the plugin wants to share the readme file as the description of the plugin for users (much like on wordpress.org, except in this case we are looking for a readme.md file instead of readme.txt).

This branch then parses the readme file into sections split on <h2> using those headlines as the tab labels. Because of a few oddities of WordPress CSS, I had to introduce a github-updater.css file to override certain spacing and coloring issues.

The end result is very much like what is done on wordpress.org, except that we leave GitHub/Bitbucket plugin authors in the comfort of familiar markdown files. I also believe that this method actually introduces no new transient issues since in the case of missing changelogs it just substitutes retrieval of the readme for the former retrieval of the changelog.

I've only tested this with Bitbucket, but I kept as much of the code as possible in common across the two repo types, so I am pretty sure it will work with GitHub as well. All you need to do is rename (or remove) your changes.md file to force the parsing of the readme.md instead.

@afragen
Copy link
Owner

afragen commented Apr 12, 2015

Thanks so much for your interest in this. I will be happy to review your branch. Please know that it may take time for me to review this.

@afragen
Copy link
Owner

afragen commented Apr 21, 2015

At some point I can see adding a function to parse a WordPress-style readme.txt, but I think using https://github.com/markjaquith/WordPress-Plugin-Readme-Parser would work better.

@efc
Copy link
Author

efc commented Apr 21, 2015

@afragen, I can see the value in that. However, the WordPress-Plugin-Readme-Parser route is also considerably more complicated than what I've proposed. It expects the MD code in a readme.txt file which is a little odd, since that is not how readme.txt is used on wordpress.org.

I also believe that it would not have to be either/or. If you implement my simpler proposal now, there is nothing that prevents you from later adding the readme.txt parsing and simply overriding the readme.md parsing when a readme.txt is available.

I am disappointed that better descriptions won't be coming to github-updater sooner. I look forward to them whichever course you take. Let me know if I can help.

@afragen
Copy link
Owner

afragen commented Apr 21, 2015

@efc the value in using the official readme.txt route is that it's standardized. It would clearly be and either/or as I wouldn't expect a user to have either a readme.txt or a CHANGES.md file.

The description comes from the Description header. From the best of my knowledge there's no size limit, though parsing a = Descripion = field in a readme.txt certainly lends for being more verbose and descriptive.

It expects the MD code in a readme.txt file which is a little odd, since that is not how readme.txt is used on wordpress.org.

I think this is exactly how how it works on .org. I have several plugins on .org and that's where the info is parsed from.

I actually have been meaning to add this. I don't think it should be overly complicated but I just haven't found the free time to start. It's coming.

@efc
Copy link
Author

efc commented Apr 21, 2015

OK, I stand corrected. I just didn't realize that wordpress.org was using MD as the markup language in readme.txt. Thanks!

@afragen
Copy link
Owner

afragen commented Apr 21, 2015

I'm actually making pretty quick progress on this. I'll ping back here, but expect something in next release.

@afragen
Copy link
Owner

afragen commented Apr 22, 2015

@efc this one's for you. e86f116

Switch to develop branch to test. You can use the branch switcher available from the Settings to select the develop branch.

Please let me know how it's working for you.

@afragen
Copy link
Owner

afragen commented Apr 22, 2015

You must use a valid readme.txt file as expected for wp.org

@afragen afragen self-assigned this Apr 22, 2015
@pedro-mendonca
Copy link
Contributor

Should it be reading readme.txt from github repo and showing it changelog in case of changes.md not existing?

@efc
Copy link
Author

efc commented Apr 22, 2015

Wow! That was fast work! Thanks so much.

Yes, it seems to be working well in my first test. It is getting info from readme.txt and using it for everything, including the changelog, when there is no changes.md or changelog.md present.

Seeing this in action, I agree that it is a better move than playing with readme.md as I was doing. I even had markdown in my own readme.txt files, I had just written them so long ago I'd forgotten!

@pedro-mendonca
Copy link
Contributor

I'm getting this error:
Warning: array_merge() [function.array-merge]: Argument #2 is not an array in .../github-updater/src/GitHub_Updater/GitHub_API.php on line 343
And the changelog I see is still from WP official repo.

@efc
Copy link
Author

efc commented Apr 22, 2015

@pedro-mendonca ... if the plugin repo you are using for your plugin is public, could you give us a link to it? I'd like to see the error in action and the files that generate it.

@afragen ... I've noticed that my Installation section went missing. Looking at the code I see that Installation and Screenshots seem to be intentionally dropped. This is not the behavior for plugins from wordpress.org, so I don't think it is wise to delete these sections with GitHub Updater. I often look at Screenshots when evaluating an update, for example. Can we have those back?

@pedro-mendonca
Copy link
Contributor

I have two exemples:

https://github.com/ruhanirabin/WP-Optimize
https://wordpress.org/plugins/wp-optimize/

https://github.com/qTranslate-Team/qtranslate-x
https://wordpress.org/plugins/qtranslate-x/
In both of them is easy no notest the changelog is different WP/GitHub

The error I've placed above is just one line.
A lot of them are ocurring below that.

@efc
Copy link
Author

efc commented Apr 22, 2015

@pedro-mendonca ... I think I see the problem, it looks like the new readme parser only works properly with the == style section breaks. While using the ## style breaks as you have is perfectly OK with wordpress.org, it is not working with this parser. My guess is that this is leading to a variety of problems later in the processing of the plugin by GitHub Updater.

@afragen
Copy link
Owner

afragen commented Apr 22, 2015

Should it be reading readme.txt from github repo and showing it changelog in case of changes.md not existing?

@pedro-mendonca yes, that is the intended behavior.

I'm getting this error:
Warning: array_merge() [function.array-merge]: Argument #2 is not an array in .../github-updater/src/GitHub_Updater/GitHub_API.php on line 343
And the changelog I see is still from WP official repo.

I think I know where that's coming from. Let me fix and let me know if it's gone.

@afragen ... I've noticed that my Installation section went missing. Looking at the code I see that Installation and Screenshots seem to be intentionally dropped. This is not the behavior for plugins from wordpress.org, so I don't think it is wise to delete these sections with GitHub Updater. I often look at Screenshots when evaluating an update, for example. Can we have those back?

The difference is that the plugin is already installed so any information on installation is unnecessary. Screenshots have been removed as they don't display properly and I don't really wish to take the time right now to fix.

@afragen
Copy link
Owner

afragen commented Apr 22, 2015

I'm getting this error:
Warning: array_merge() [function.array-merge]: Argument #2 is not an array in .../github-updater/src/GitHub_Updater/GitHub_API.php on line 343

This should now be fixed. Please confirm.

@afragen
Copy link
Owner

afragen commented Apr 22, 2015

And the changelog I see is still from WP official repo.

Changing the View details for a GitHub sourced repo only works if the Plugin URI === GitHub Plugin URI. Unfortunately, at this time there's no way to work around this issue and the details are pulled from the default method (from .org). You would have to change the Plugin URI header for this function to work. Sorry.

@efc
Copy link
Author

efc commented Apr 22, 2015

I have created two do-nothing plugins to test the readme.txt parsing. On bitbucket you will find efc/test-readme-traditional and efc/test-readme-markdown to test both the == and ## flavor of WordPress readme.txt. Both are working fine right now.

@pedro-mendonca
Copy link
Contributor

If the problem is the plugin URI, than that's out of most users control.
Probably that URI will be either their WP repo or some developer website.
Thanks anyway.

The error problem is solved 👍

@afragen
Copy link
Owner

afragen commented Apr 22, 2015

If the problem is the plugin URI, than that's out of most users control.
Probably that URI will be either their WP repo or some developer website.

There is also a secondary issue. If the plugin branch is master and it's also on .org, the plugin preferentially pulls from .org and yes it is outside of the user's control for other people's plugins.

@pedro-mendonca
Copy link
Contributor

👍

@afragen afragen closed this as completed Apr 22, 2015
@afragen
Copy link
Owner

afragen commented Apr 24, 2015

Changing the View details for a GitHub sourced repo only works if the Plugin URI === GitHub Plugin URI. Unfortunately, at this time there's no way to work around this issue and the details are pulled from the default method (from .org). You would have to change the Plugin URI header for this function to work. Sorry.

I have found a way to work around this issue.

@efc
Copy link
Author

efc commented Apr 24, 2015

Great to see this roll out in the 1.4.0 update. Thanks, @afragen!

However, I am finding that the release does not behave quite as the last development version I tested. Everything works fine for my efc/test-readme-traditional plugin, but my efc/test-readme-markdown plugin does not show any "view details" link on the plugins list, showing "visit plugin site" instead.

Seems the == and ## forms of readme.txt are being treated differently after all?

@afragen
Copy link
Owner

afragen commented Apr 24, 2015

Let me test a sec.

@afragen
Copy link
Owner

afragen commented Apr 24, 2015

The header for test-readme-markdown is wrong. It is the same as test-readme-traditional. Fix that and I'm betting it works.

@efc
Copy link
Author

efc commented Apr 25, 2015

Oops, you are right, the lack of viewing details was a header problem with the plugin. Sorry about that. But I've fixed that and found an even more serious problem. Now some PHP notices are appearing on every page and the details, when viewed, are the default and not those from the readme.

Notice: Undefined index: sections in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/src/GitHub_Updater/Bitbucket_API.php on line 322

Notice: Undefined index: tested_up_to in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/src/GitHub_Updater/Bitbucket_API.php on line 323

Notice: Undefined index: requires_at_least in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/src/GitHub_Updater/Bitbucket_API.php on line 324

Notice: Undefined index: donate_link in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/src/GitHub_Updater/Bitbucket_API.php on line 325

Notice: Undefined index: contributors in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/src/GitHub_Updater/Bitbucket_API.php on line 326

Warning: Invalid argument supplied for foreach() in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/src/GitHub_Updater/Plugin.php on line 253

This problem only appears when the plugin with the fully markdown form of readme.txt is installed.

@afragen
Copy link
Owner

afragen commented Apr 25, 2015

This problem only appears when the plugin with the fully markdown form of readme.txt is installed.

I guess that's the answer then. You must have a valid readme.txt file.

@efc
Copy link
Author

efc commented Apr 25, 2015

Well, yes, I suppose. However this readme.txt file does pass the WordPress validation with flying colors. I can't find this ## form documented, I only noticed it because of @pedro-mendonca's report above. But it does seem common enough. Even if it is not properly parsed, it would be better if the failure was more graceful, without the multiple PHP notices.

@efc
Copy link
Author

efc commented Apr 25, 2015

I wonder... There must be something in the WordPress core that parses these files for the wordpress.org plugins. I'll have to dig for it this weekend. I wonder if it could be leveraged here instead of using an outside parser?

@afragen
Copy link
Owner

afragen commented Apr 25, 2015

@efc this is the parser they use. They might have made a few additions, however, they haven't put them back in.

@efc
Copy link
Author

efc commented Apr 25, 2015

While the validator seems to accept ## readme files, WordPress itself seems to ignore them completely and put up the "Visit plugin site" instead of "View details" when they are present. At least, if I remove the Bitbucket URI from my markdown test so that WP processes the plugin entry on the plugins page, that is what happens. So I think you are right to not worry about this case.

However, even in the case of a malformed readme.txt I don't think you want GitHub Updater throwing PHP notices when define('WP_DEBUG', true) is set. I've suggested a few changes to the API files to deal with the notices I was seeing. One more change to vendor/parse-readme.php resolves a notice that was even being seen with the == style readme when it was missing an upgrade notice. These are in the "develop" branch at "efc/github-updater".

I am a git novice, so I don't know if I'm doing the forking/fetching/rebasing stuff right. At least I can't hurt anything in my own fork!

@afragen
Copy link
Owner

afragen commented Apr 25, 2015

I'll take a look and I agree the errors should be able to be mitigated.

@afragen
Copy link
Owner

afragen commented Apr 26, 2015

@efc latest commit fd42bc4 should fix all the PHP notices.

@efc
Copy link
Author

efc commented Apr 27, 2015

Yes, that commit does fix the PHP notices being thrown from your code. However there is still one notice being thrown by the parse-readme.php script whenever a the "Check Again" button is pressed on the update page with a poorly formatted readme.txt file present:

Notice: Undefined offset: 1 in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/vendor/parse-readme.php on line 159

My suggested changes included a fix for that one too, but maybe you are trying not to modify that vendor code.

In any case, at least that notice only shows up when checking for new versions and does not become visible to most users.

@afragen
Copy link
Owner

afragen commented Apr 27, 2015

maybe you are trying not to modify that vendor code.

I'm really not trying to fix the vendor code for edge cases. I did fix it for a few errors but they were error in showing in standard readme.txt

@efc
Copy link
Author

efc commented Apr 27, 2015

That's up to you. As I read the code, it is simply wrong as written, which is why it generates the notice. The code verifies the length of an array, then seeks to use an element one further than the length it verified. Sure, it is only showing up as a notice on this edge case (so far), but it is still a mistake.

@afragen
Copy link
Owner

afragen commented Apr 27, 2015

Yes, that commit does fix the PHP notices being thrown from your code. However there is still one notice being thrown by the parse-readme.php script whenever a the "Check Again" button is pressed on the update page with a poorly formatted readme.txt file present:

Notice: Undefined offset: 1 in /home4/clstorg/public_html/wp/wp-content/plugins/github-updater/vendor/parse-readme.php on line 159

FWIW, I have both of your test plugins installed locally and I don't see this particular PHP notice on the current develop branch.

@efc
Copy link
Author

efc commented Apr 28, 2015

That's interesting. Do you have define('WP_DEBUG', true) set? This notice only appears when debugging.

@afragen
Copy link
Owner

afragen commented Apr 28, 2015

That's interesting. Do you have define('WP_DEBUG', true) set? This notice only appears when debugging.

Always and using PHPStorm, xdebug and Query Monitor plugin.

@afragen
Copy link
Owner

afragen commented Apr 28, 2015

However there is still one notice being thrown by the parse-readme.php script whenever a the "Check Again" button is pressed on the update page with a poorly formatted readme.txt file present:

Does the readme.txt in question pass the validator? Which plugin is it associated with?

@efc
Copy link
Author

efc commented Apr 28, 2015

Both the readme's in test-readme-markdown and test-readme-traditional pass the validator.

Of course, at this point we expect markdown's readme.txt to not work due to the ## demarcation of the sections, even though the validator has no problem with that. Oddly enough, on more hunting through backtraces, it looks like the markdown readme is not causing the notice in the parse-readme.php.

That notice is being thrown when the parser tries to read the traditional == readme file. The trouble with this file was that though it was passing the validator, it actually did not properly format the "upgrade notice" section. The parser expects that section to be a series of <h4> headlines followed by 300 character or less notices. My old traditional readme only had the notice, not the headline. Note that this notice was only seen when on the updates page in the dashboard (update-core.php), not anywhere else.

The bottom line is that the parse-readme.php parser is more fragile than whatever is built into WordPress core, since the core is not throwing notices. I have no idea where the parser used by the cord lives, so I have not inspected it.

I've updated the test-readme-traditional plugin with an even more correct readme.txt file which no longer forces this notice from PHP. However, I still think it would be useful to fix the parse-readme.txt file to check for what it really wants when processing the "upgrade notice," which is a set of two array elements, not just a single element.

@efc
Copy link
Author

efc commented Apr 28, 2015

BYW, @afragen, in case I have not been clear about this... github-updater is super useful and I am really psyched that you included the readme parsing. This last issue is minor, only evident when debugging, only present when the readme does not meet the documented spec, and a problem with a vendor script, so I don't mind at all if you move on to more important issues! I'm happy with the current state of affairs. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants