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

Use constants in AMP_Allowed_Tags_Generated #4536

Closed
schlessera opened this issue Apr 6, 2020 · 3 comments
Closed

Use constants in AMP_Allowed_Tags_Generated #4536

schlessera opened this issue Apr 6, 2020 · 3 comments
Assignees
Labels
Groomed P1 Medium priority Punted Tech Debt Deprecations, inefficiencies, code health Validation WS:Perf Work stream for Metrics, Performance and Optimizer

Comments

@schlessera
Copy link
Collaborator

Feature description

While trying to investigate the reason for #4465, I noticed that the constants being used within the protoascii files are reverted to meaningless scalar values in the generated PHP files.

The PHP files should make use of defined constants like the ones defined in AmpProject\Layout or similar, so that it becomes much easier to reason about the generated rules.

Example before (extract):

'amp-audio' => array(
array(
    'tag_spec' => array(
        'amp_layout' => array(
            'defines_default_height' => true,
            'defines_default_width' => true,
            'supported_layouts' => array(
                2,
                3,
                1,
            ),
        ),
    ),
),

Example after (extract):

'amp-audio' => array(
array(
    'tag_spec' => array(
        'amp_layout' => array(
            'defines_default_height' => true,
            'defines_default_width' => true,
            'supported_layouts' => array(
                Layout::FIXED,
                Layout::FIXED_HEIGHT,
                Layout::FILL,
            ),
        ),
    ),
),

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • All constants defined in protoascii should translate to constants defined in PHP.

Implementation brief

QA testing instructions

Demo

Changelog entry

@schlessera schlessera added Validation Tech Debt Deprecations, inefficiencies, code health labels Apr 6, 2020
@westonruter
Copy link
Member

Doing this would be complicated given the current spec generator, which takes a data structure and then just does var_export(). This may make most sense to tackle as part of #3875.

@amedina amedina added the P1 Medium priority label May 14, 2020
@kmyram kmyram added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 5, 2020
@westonruter
Copy link
Member

Being worked on as part of #3730.

@kmyram kmyram added this to the v2.1 milestone Feb 2, 2021
@kmyram kmyram added the Groomed label Feb 2, 2021
@westonruter westonruter modified the milestones: v2.1, v2.2 Feb 11, 2021
@westonruter
Copy link
Member

Implemented in ampproject/amp-toolbox-php#100

@westonruter westonruter removed this from the v2.2 milestone Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Groomed P1 Medium priority Punted Tech Debt Deprecations, inefficiencies, code health Validation WS:Perf Work stream for Metrics, Performance and Optimizer
Projects
None yet
Development

No branches or pull requests

4 participants