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

Turn TagWithExtensionSpec into an abstract class #358

Merged
merged 3 commits into from
Sep 20, 2021
Merged

Conversation

schlessera
Copy link
Collaborator

This PR fixes lots of PHPStan issues.

One of the main changes is that it turns the TagWithExtensionSpec interface into an abstract class that extends from Tag. This solves a few inconsistencies and gets rid of the ExtensionSpec trait.

@schlessera schlessera added Tech Debt Deprecations, inefficiencies, code health Validator labels Sep 20, 2021
@schlessera schlessera added this to the 0.8.0 milestone Sep 20, 2021
Comment on lines +118 to +121
// There is currently a mismatch between data in the validator spec and data in the bundle config for the targeted
// release. Therefore, we fall back to main branch for now.
// Original code: "$latest_bundle_config_url = sprintf($bundle_config_url, $latest_release['name']);".
$latest_bundle_config_url = sprintf($bundle_config_url, 'main');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@westonruter The code you've built to pick the correct release references a release that does not yet include some of the data that the validator JSON from the CDN needs. Assuming the CDN only returns actual release specs would mean the code in here produces outdated release labels...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've hit this issue now with the amp-story-captions extension which is included in the validator.json from the CDN. But the bundle config did not yet include it, meaning I ended up with a LATEST_VERSION of null.

@schlessera schlessera merged commit 6398c17 into main Sep 20, 2021
@schlessera schlessera deleted the fix/phpstan-issues branch September 20, 2021 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech Debt Deprecations, inefficiencies, code health Validator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant