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.

Sign up for GitHub

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

Jump to bottom

Use class constants instead of classes as string literals #6545

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Use class constants instead of classes as string literals #6545

merged 1 commit into from
Aug 17, 2021

Conversation

westonruter
Copy link
Member

Summary

Since we now require PHP 5.6+, we can make use of the ::class constant instead of using string literals. This will make it easier to catch bugs (as this ended up doing) and make it easier to do refactorings.

Checklist

  • My code is tested and passes existing tests.
  • My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

@westonruter westonruter added the Tech Debt Deprecations, inefficiencies, code health label Aug 16, 2021
@westonruter westonruter added this to the v2.2 milestone Aug 16, 2021
@westonruter westonruter requested a review from pierlon August 16, 2021 22:21
westonruter
westonruter commented Aug 16, 2021
View reviewed changes
includes/class-amp-theme-support.php
__( 'Embed Handler (%1$s) must extend `%2$s`', 'amp' ),
esc_html( $embed_handler_class ),
'AMP_Embed_Handler'
AMP_Base_Embed_Handler::class
Copy link
Member Author

Choose a reason for hiding this comment

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

Bugfix!

westonruter
westonruter commented Aug 16, 2021
View reviewed changes
includes/templates/class-amp-content.php
__( 'Embed Handler (%1$s) must extend `%2$s`', 'amp' ),
esc_html( $embed_handler_class ),
'AMP_Embed_Handler'
AMP_Base_Embed_Handler::class
Copy link
Member Author

Choose a reason for hiding this comment

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

Bugfix!

@github-actions
Copy link
Contributor

Plugin builds for 7b85909 are ready 🛎️!

@westonruter westonruter enabled auto-merge August 16, 2021 22:26
pierlon
pierlon approved these changes Aug 17, 2021
View reviewed changes
Copy link
Contributor

@pierlon pierlon left a comment

Choose a reason for hiding this comment

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

Interesting! It's great that you caught those undefined classes before they became reported bugs.

Was there any fancy way you used to find these strings or was it a search/replace?

@westonruter westonruter merged commit 24923a4 into develop Aug 17, 2021
@westonruter westonruter deleted the update/class-constant-usage branch August 17, 2021 04:02
@westonruter
Copy link
Member Author

Was there any fancy way you used to find these strings or was it a search/replace?

I used a regex search and replace, essentially: s/'(AMP_(\w+[a-z]|HTTP))'/$1::class/

@bartoszgadomski
Copy link
Contributor

I'm not sure how to QA this ticket; if there would be any issues caused by this change, we'd notice this either during running the automated tests or while developing the other stuff. Do we have any manual test scenarios defined somewhere for tickets like this one?

I did run smoke tests and didn't notice any issues, so I think we can mark it as QA passed.

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierlon pierlon pierlon approved these changes

Assignees

@bartoszgadomski bartoszgadomski

Labels
Changelogged Whether the issue/PR has been added to release notes. Tech Debt Deprecations, inefficiencies, code health
Projects
None yet
Milestone
v2.2
Development

Successfully merging this pull request may close these issues.
3 participants
@westonruter @bartoszgadomski @pierlon