-
Notifications
You must be signed in to change notification settings - Fork 384
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
Update amp-toolbox-php to 0.6.0 (almost) and update error page to support fatal errors (in PHP 7+) #6421
Conversation
Plugin builds for 6387eca are ready 🛎️!
|
sprintf( | ||
/* translators: %s is the AMP support forum URL. */ | ||
__( 'If you get stuck, you may want to share any details in a new topic on the plugin\'s <a href="%s" target="_blank" rel="noreferrer noopener">support forum</a>.', 'amp' ), | ||
esc_url( __( 'https://wordpress.org/support/plugin/amp/', 'amp' ) ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually want this link to be translatable? It could potentially allow an attacker to send the user to a "fishing support forum" where they post their logs to a third-party... Yes, it is far-fetched, I know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The URL was made translatable in #6320 as well. There are these instances of translatable URLs:
includes/options/class-amp-options-manager.php
470: esc_url( __( 'https://wordpress.org/support/article/why-should-i-use-https/', 'amp' ) )
assets/src/components/error-screen/index.js
49: __( 'https://wordpress.org/support/plugin/amp/', 'amp' ),
assets/src/settings-page/analytics.js
26: __( 'https://wordpress.org/plugins/google-site-kit/', 'amp' ),
27: __( 'https://developers.google.com/analytics/devguides/collection/amp-analytics/', 'amp' ),
213: __( 'https://amp.dev/documentation/components/amp-analytics/', 'amp' ),
215: __( 'https://amp-wp.org/documentation/getting-started/analytics/', 'amp' ),
220: __( 'https://amp.dev/documentation/guides-and-tutorials/optimize-and-measure/configure-analytics/analytics-vendors/', 'amp' ),
assets/src/settings-page/paired-url-structure.js
337: __( 'https://amp-wp.org/?p=10004', 'amp' ),
src/Admin/SiteHealth.php
248: esc_url( __( 'https://www.php.net/manual/en/book.curl.php', 'amp' ) ),
src/DevTools/ErrorPage.php
277: esc_url( __( 'https://wordpress.org/support/plugin/amp/', 'amp' ) )
src/LoadingError.php
47: esc_url( __( 'https://wordpress.org/support/plugin/amp/', 'amp' ) )
Granted, we aren't going to have alternative support forums so I'm fine with removing the translation in both places.
cc @delawski
…is missing in amp-toolbox-php" This reverts commit 5968fe9.
…port fatal errors (in PHP 7+) (#6421)
Summary
Fix fatal error now raised inAMP_Object_Sanitizer
sinceTag::OBJECT
no longer exists in amp-toolbox-php.Fatal Error Handling
Forum Link
Checklist