Improve handling of latest extension versions in spec

ampproject / amp-toolbox-php

AMP Optimizer PHP library

Apache License 2.0

73 stars 25 forks source link

Improve handling of latest extension versions in spec #232

Open schlessera opened 3 years ago

schlessera commented 3 years ago

Multiple TODOs are still open in the ExtensionSpec template. These need to be resolved be generally improving the handling of latest versions of extensions.

[ ] The spec will need to be adapted to be informed by https://github.com/ampproject/amphtml/blob/main/build-system/compile/bundles.config.extensions.json, where the latestVersion is stored with the spec (and the highest version is not always the same as latest), as well as any additional Bento metadata.
[ ] There is one case where an extension lacks a 'latest' version, but maybe not for long. See https://github.com/ampproject/amphtml/pull/34636.
[ ] Why isn't the sorting done at ingestion time? Then getLatestVersion() could just be doing something like return self::LATEST_VERSION.

https://github.com/ampproject/amp-toolbox-php/blob/c680d60e9cf03c0e4c8a0f0ff4b3b3c662058eea/bin/src/Validator/SpecGenerator/Template/ExtensionSpec.php#L29-L67

westonruter commented 3 years ago

The spec will need to be adapted to be informed by https://github.com/ampproject/amphtml/blob/main/build-system/compile/bundles.config.extensions.json, where the latestVersion is stored with the spec (and the highest version is not always the same as latest), as well as any additional Bento metadata.

See https://github.com/ampproject/amphtml/pull/34838 for the latest on where the Bento information is stored.

westonruter commented 3 years ago

In order to determine which Bento versions are stable enough to be used, we have to cross-reference to make sure the version is among the versions which are present in the validator spec. See https://github.com/ampproject/amp-wp/pull/6373.

schlessera commented 3 years ago

@ediamin Some of the above TODOs should already be solved by https://github.com/ampproject/amp-toolbox-php/pull/297. Can you check in detail what was done, and what is still missing to update the issue description?

ediamin commented 3 years ago

Here are my findings about the current state of this issue:

The spec has adapted the bundles.config.extensions.json well from the ampproject/amphtml project. The latestVersion is stored with the spec as well as additional Bento metadata. It was handled in #297 .
The ampproject/amphtm#34636 PR is still open and need to be merged.
In PR#358 TagWithExtensionSpec was introduced. In which getLatestVersion() simply returns self::LATEST_VERSION.

westonruter commented 3 years ago

The spec has adapted the bundles.config.extensions.json well from the ampproject/amphtml project. The latestVersion is stored with the spec as well as additional Bento metadata. It was handled in #297 .

The Bento version is also now indicated in the validator spec via a bento_supported_version attribute: https://github.com/ampproject/amphtml/pull/35757. The plugin is still using the version data in bundles.config.extensions.json, but I believe we could skip consuming that JSON file now that the validator has the data as well.