Test my user documentation + make a new JSON card

philwareham commented 4 years ago

Hi @Bloke @petecooper @bloatware @jools-r - hope you are all well?

We are getting closer to launching a new plugins site 🎉! To that end, can you please (when you get a spare moment) peruse the README file of this repo for understandability, typos and/or missing info and let me know if you spot anything amiss.

If you could also attempt to fill out a new plugin JSON card for an existing Textpattern plugin that doesn't yet appear in the library-of-plugins that would be wonderful. I think you all have direct write access to this repo but if not then just shout and I'll provide.

Once the above is done successfully, I can demonstrate the process of how these cards are then used to power the plugins site. Then we can all retire as millionaires¹.

Thanks for your time.

¹ payment not guaranteed.

philwareham commented 4 years ago

@petecooper thanks for the smd_query card. I've created the corresponding page stub for this on the plugins website as follows:

https://plugins.textpattern.com/plugins/smd_query

If you can add a 'homepage' entry in the 'repos' section of that card too, you'll then see that appear in the plugins site as a link on the sidebar when it's been committed to this repo and the latest sync has taken place.

petecooper commented 4 years ago

Yep - that's my plan, I'll drip feed in some bits over the day to check it's all playing nicely.

philwareham commented 4 years ago

Great! I'll eventually document the process of getting a new plugin into the plugins site and the process for validating a plugin for a particular Textpattern version. Plan is that validated plugins (and only validated ones) will be used by future Textpattern to allow users to auto-update their plugins via a new JSON card, that is generated by the plugins site itself (a kind of amalgamation of these JSON cards plus the validation flags and endpoints). It's all fairly simple to generate though, so don't worry!

jools-r commented 4 years ago

Great work Phil + Stef (and anyone else involved)!

I added a simple library card for one of my plugins. What I wasn't quite sure about:

For downloadUrlTxt I looked at the other examples and used the link under "Releases". If you navigate to the file and view the raw version from GitHub, it gives you a different link. Is there any preference or is it irrelevant.
For downloadUrlPhp, I again followed the example of the others, but the archive is actually the zip file to the entire repo and not just the .php file of the plugin. Was that correct, or should it be just the php file? Does that perhaps need clarification?

How do you get the actual description of the plugin into the directory? Does that have to be added manually after the event? Could/should it also be part of the library card?

Bloke commented 4 years ago

downloadUrlPhp should be to the raw .php Template file. Only if the plugin is an officially-bundled .zip file (made up of individual file components, like abc_prosemirror did) should it point to that zip file.

I've not tested trying to import a GitHub 'zip' archive - the one available when you click Clone or Download - via the Plugins panel. It might install as a .zip directly, depending on how the author has set the plugin's file structure out.

jools-r commented 4 years ago

Thanks. Ok then, that needs clarifying in the docs, also that for downloadUrlPhp, it's the “raw” php link that's needed and not just GitHub’s display of the php file.

Also, again just for clarification, for downloadUrlTxt does it matter if the txt link is a:

https://raw.githubusercontent.com/{github-user}/{prefix_plugin_name}/{branch}/{prefix_plugin_name}-{semver}.txt – which displays in the browser.
https://github.com/{github-user}/{prefix_plugin_name}/releases/download/{semver}/{prefix_plugin_name}-{semver}.txt – which downloads directly from the browser.

I've updated the library card regarding the php source. The other library cards may also need updating where the zip has been specified, though maybe the rah_ plugins are a special case as Jukka's own plugin installer splits the code across multiple php files.

philwareham commented 4 years ago

downloadUrlPhp should be to the raw .php Template file.

Ok, thanks for clarifying. I'll amend the cards we've done where necessary and also try to document this in an understandable way. I think we can browse the filetree at a given release tag so that would be where we would need to pick up the php file from.

To be honest I was hoping we could just download the release zip and the system could figure out what it needs, but that's probably not going to work in hindsight.

Also, again just for clarification, for downloadUrlTxt does it matter if the txt link is ...

I'd say option two, as preference. There may be cases where a GitHub user doesn't tag their plugins with releases, in which case we maybe would need to use option one. Haven't investigated yet.

philwareham commented 4 years ago

Actually I don't think we can use raw because they don't download (only show in browser) - so we do need to download the release zip I guess? I don't see that as an issue when downloading manually from the plugins site but it might well have an impact on auto-installing directly in the Textpattern Plugins panel.

petecooper commented 4 years ago

Actually I don't think we can use raw because they don't download (only show in browser)

~Any scope for PHP curl since we already have it on the sys reqs?~

Edit: actually, we don't - ignore me. But the update check stuff happens with curl, right?

philwareham commented 4 years ago

Any scope for PHP curl since we already have it on the sys reqs?

That would solve it on core, if available, yes. Not sure how that would impact downloading from the site itself though (if we link to raw plugins on GitHub it would probably show them in browser not download).

For reference: curl -o filename raw-link-to-file

Or probably better use file_get_contents.

Bloke commented 4 years ago

From Txp and the Plugin repo's perspective, it doesn't matter if you link to raw. or the other one in the branch. Just as long as it's the file and not how GitHub represents it we'll be fine. For the case of instructions though, I'd say pick one - and yeah, the second one is simpler.

To be honest I was hoping we could just download the release zip and the system could figure out what it needs, but that's probably not going to work in hindsight.

That would be nice. I just tried it and it didn't work right now but it could be made to work, IF there's a manifest in the package.

At the moment the main problem is that the downloaded file is tagged with the branch name. So the zip file is something like smd_thumbnail-master or jcr_link_custom-4.8. As it stands, we assume that the filename of the zip is just the plugin name so we unzip it in its own subdirectory.

To facilitate this, the Plugins panel looks at the filename and assumes the zip file structure follows:

abc_plugin/abc_plugin.php
abc_plugin/help.textile
...

In GitHub download zips, that's actually:

abc_plugin-branch/abc_plugin.php
abc_plugin-branch/README.textile
...

That means when looking inside the zip for the plugin .php file and the help to display them in the Preview step, the uploader can't find the files. They don't match the expected structure.

So we need to be more clever about this. A few ways we could do it:

1) Assume that anything after a hyphen is a branch name and strip it off in the code (if present) to get the "real" plugin name. Then use the actual filename as the expected dir name inside the archive (instead of assuming it's always just the plugin name). That's kinda of fraught with a little bit of danger, but could be used as a fallback if... 2) ... inside the archive is a manifest.json file. I propose that this houses a record containing the actual plugin name, its version, type, etc AND a link to any special supplementary files. In our case, this is:

help file (which defaults to help.textile if not specified in manifest).
textpack(s) (which defaults to a single textpack.txp file containing all strings in all languages, though it could be held in separate files inside a lang directory. Perhaps we could support that by combining them into one on installation).
plugin data (to go in the plugin's data column. This default to data.txp unless otherwise instructed in manifest. Again, if it points to a subdir or multiple files, we could concatenate them prior to installation).

In short, yes we could handle GitHub .zip files if we tweak the uploader to make fewer assumptions about its structure and defer structural information to the author via an implicit manifest.json file that we define as having keys that we look for.

The downside is that if the manifest is wrong, well the plugin won't install. Down to the author to fix. If the manifest is missing or has missing entries, we'll have to do our best to sniff out the files we need based on some conventions that repos such as GitHub employ.

EDIT: I don't want to pander to one particular vendor, so the manifest gives us a nice way of letting plugin authors help us to provide a seamless installation experience for users (which plugin authors want, right, with minimal extra work on their part?). Plus, the same manifest helps define stuff for Packagist, doesn't it, so a doubly good reason for using it?

Howzat?

Bloke commented 4 years ago

Note that we already support 'help' in the manifest.json file. Here's an example from smd_thumbnail:

{
  "name": "smd_thumbnail",
  "description": "Multiple image thumbnails of arbitrary dimensions",
  "version": "0.5.3",
  "type": 5,
  "author": "Stef Dawson",
  "author_uri": "https://stefdawson.com/",
  "order": 5,
  "flags": 2,
  "help": {"file" : ["./README.textile"]}
}

All we really need to do is come up with a structure for Textpacks and (maybe) data files and hold that manifest aloft as a shining example of how to do it "right". Then I'll code the uploader on the Plugins panel to read it so we can then use the 'php' endpoint URL as the release's .zip file and be done with it.

philwareham commented 4 years ago

The manifest.json route would be my preferred route too. It's a pretty widely accepted way of describing a package. We can always encourage plugin authors to use it and send PRs where it's not currently in place. Any plugin that doesn't have a manifest I guess would be excluded from auto-updates (maybe we need to flag whether a manifest is available in these JSON cards or on the plugins site?).

Bloke commented 4 years ago

We can always encourage plugin authors to use it and send PRs where it's not currently in place.

Definitely.

Any plugin that doesn't have a manifest I guess would be excluded from auto-updates (maybe we need to flag whether a manifest is available in these JSON cards or on the plugins site?).

That's a nice end goal. In practice if the endpoint is a .zip file, somewhere it's going to have to be expanded to read the contents and then decide if it's valid or not. It puts load on the Plugin repo to do that, but I'd rather do it there than have everyone's Plugins panels have to check to see if all plugins are valid. Lots of network traffic!

For now, let's assume:

Links to .txt files work as-is, BASE64.
Links to .php files are to the actual raw.githubusercontent.com or the .php file in a release. Doesn't matter. They can either be straight PHP or a plugin template.
Links to .zip files are all going to have valid manifests. When dragged into the upload box or (eventually) offered as an auto-update, we can decide whether to proceed based on how well the .zip bundle conforms to our expected structure. If it has a manifest, great. If it doesn't, we'll best guess. And if it looks trash, we'll reject it there before the verify step.

Longer term, we can either burden the end user with failings on the plugin author's part or, if we can reasonably decode the plugin's intent, maybe we let it proceed. Without a manifest, some stuff might not be installed if it doesn't conform to a standard naming convention so we need to weigh up whether that's a risk we're willing to expose to end users.

If not, then yes, we should find a way to flag this card as "not upgradable" and (perhaps) show this to people on their Plugins panels. If they see a plugin that they really, really want is mangled in some way, it might spur them into action to either contact the author or pop into the forum to see if anyone can patch or maintain it.

Bloke commented 4 years ago

How about this:

{
  "name": "smd_thumbnail",
  "description": "Multiple image thumbnails of arbitrary dimensions",
  "version": "0.5.3",
  "type": 5,
  "author": "Stef Dawson",
  "author_uri": "https://stefdawson.com/",
  "order": 5,
  "flags": 2,
  "help": {"file" : ["./README.textile"]},
  "code": {"file" : ["./smd_thumbnail.php"]},
  "textpack": {"file" : ["./textpack.txp"]},
  "data": {"file" : ["./data.txp"]}
}

Those last three blocks, however, could be:

}
...
  "code": {"file" : [
        "./src/Functions.php",
        "./src/Index.php"
  ]},
  "textpack": {"file" : [
    {
      "en": "./lang/en.textpack",
      "de": "./lang/de.textpack",
      "es": "./lang/es.textpack"
    }
  ]},
  "data": {"file" : [
    {
      "lib": "./data/library.js",
      "style": "./data/pretty-file.css"
    }
  ]}
}

Any better structures?

EDIT by Phil: added code entries as discussed below.

philwareham commented 4 years ago

How about this

Fine by me. What are your thoughts on a phpHasManifest JSON entry on the cards? Just thinking that saves the download of zip file only to find it hasn't got the expected file within. The core system would have to read the JSON card anyway prior to any download so the network traffic is going to happen regardless.

That only affects PHP download though, TXT download doesn't need a manifest. I can code this condition into the plugin site's JSON output.

Bloke commented 4 years ago

phpHasManifest works for me.

philwareham commented 4 years ago

Just re-looking at @bloke the manifest, maybe just change the final entry from data to code so that it matches Jukka's the established entries of manifest files in his plugins: https://github.com/gocom/rah_replace/blob/master/manifest.json

Bloke commented 4 years ago

We don't have a code entry. It's currently assumed to match the name of the plugin. But you're right, we should have that in case people put their code in multiple chunks or in a subdir. The data block is for supplemental files that get bundled into the plugin's data column in the txp_plugin table.

philwareham commented 4 years ago

Ah, OK, but yes I think we should include code as a recommendation.

Rationale and use in core auto-update:

Manifest doesn't need to contain a code entry when there is a single PHP file in the root with filename that matches plugin name.
Manifest does need to contain a code entry when there are multiple PHP files or one of more PHP files stored in a subdirectory (i.e. how Jukka does it).

Bloke commented 4 years ago

Perfect!

philwareham commented 4 years ago

@bloke got another usage case where we will probably need a JSON entry in cards: wet_quickopen plugin requires wet_peex- how would we manage that?

Bloke commented 4 years ago

Oooh, dependencies. Good thinking. Is there anything we can cadge from composer, without the awkward syntax?

philwareham commented 4 years ago

Seems to be a standard syntax:

  "require": [
    "wet_peex": "1.0.0",
    "something_else": "0.8.1"
  ]

Not sure if it would be [ ... ] or { ... } to enclose it though?

Bloke commented 4 years ago

That's cool. I'm used to seeing comparators like:

  "require": {
    "textpattern/lock": ">=4.7.0",
    "textpattern/installer": "*"
  }

(braces, not square brackets it seems)

We could support that syntax if we needed to. Doesn't make it that much trickier to implement, and is fairly beneficial.

From the plugin verify step (and hence, upgrade step), if there are unmet dependencies in your installation, I think we should highlight them but still permit installation. It's annoying to be told "you can't install this because you don't have such-and-such available" so I think a warning is sufficient and you can then fulfill that dependency yourself prior to enabling/using the plugin.

philwareham commented 4 years ago

OK, added that to the plugin in question: https://github.com/textpattern/textpattern-curated-plugins-list/commit/a69fcd389aa7d4dd0a9bb9a8c24a3bed12cf16e4

Bloke commented 4 years ago

Sweet. I'll factor that into the plugin class next time I have a stab at the uploader/importer.

philwareham commented 4 years ago

@bloke great! Can you please supply me some code to go in here that can grab any require entries (plugin name and plugin version) and put them into a variable or slug of code I can manipulate? Similar to what we have done for other parts of the JSON file. Note there may be more than one plugin required (rare but possible). Cheers!

Bloke commented 4 years ago

As a starter:

if (!empty($json->require)) {
    foreach ($json->require as $rplug => $rver) {
        $vars['require-plug-name'] = '<txp:variable name="json-require-plugin">'.txpspecialchars($rplug).'</txp:variable>';

        preg_match('/([\<\=\>]+)?([\d.]+)/', $rver, $matches);
        $matchType = 'equal';

        switch ($matches[1]) {
            case '>':
                $matchType = 'greater than';
                break;
            case '>=':
                $matchType = 'at least';
                break;
            case '<':
                $matchType = 'less than';
                break;
            case '<=':
                $matchType = 'no greater than';
                break;
            case '=':
            default:
                $matchType = 'equal';
                break;
        }

        $plain_ver = $matches[2];

        $vars['require-plug-match'] = '<txp:variable name="json-require-match">'.txpspecialchars($matchType).'</txp:variable>';
        $vars['require-plug-version'] = '<txp:variable name="json-require-version">'.txpspecialchars($plain_ver).'</txp:variable>';
    }
}

But that only works if there's one entry in the require block. If there's more than one, it'll only return the last one. Realistically, are we expecting more than one? If so, we might be once again at the limits of <txp:variable> and might have to seek a more PHPish solution.

But see if that suits your needs for now and we'll advance it if necessary.

EDIT: Also doesn't take into account alpha/beta/rc versions and stuff like that.

philwareham commented 4 years ago

Closing this issue as all stuff discussed within is now done. Any new discussions can be had in new issues. Thanks for the feedback all.

philwareham commented 4 years ago

Forum announcement about plugins site being live: https://forum.textpattern.com/viewtopic.php?id=50877

textpattern / textpattern-curated-plugins-list

Test my user documentation + make a new JSON card #7