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

Remove potential for fatals on front end. #187

Merged
merged 4 commits into from Jan 16, 2023

Conversation

peterwilsoncc
Copy link
Contributor

@peterwilsoncc peterwilsoncc commented Jan 16, 2023

Description of the Change

This ensures Simple_Local_Avatars::$avatar_ratings is always defined on the front end to account for themes using avatars.

It also fixes a deprecation warning in PHP 8.0

Closes #186

How to test the Change

  1. Login as an admin
  2. Activate the Twenty Twenty-One default theme*
  3. Author some posts as the admin
  4. Create a local avatar on the admin's profile, save
  5. Set the local avatar's rating to PG (anything other than G rated should work)
  6. Visit the front end of the site
  7. On develop you should see a fatal error, on this branch you should not.

* I was reliably able to reproduce the bug using TT1 but I think it would happen on all themes when viewing the front end while logged in due to the admin bar been displayed.

Changelog Entry

Fixed - Issue causing fatal errors when avatars used on front end of site.
Fixed - Deprecation error in admin on PHP 8.0 and later

Credits

Props @peterwilsoncc, @Rottinator.

Checklist:

  • I agree to follow this project's Code of Conduct.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests pass.

@peterwilsoncc peterwilsoncc marked this pull request as ready for review January 16, 2023 00:49
@peterwilsoncc
Copy link
Contributor Author

This could probably do with some tests but because I wasn't able to figure out how best to do so.

The ratings are always set in the tests due to these lines

$avatar_ratings = $reflection->getProperty( 'avatar_ratings' );
$avatar_ratings->setAccessible( true );
$avatar_ratings->setValue( $this->instance, array(
'G' => __( 'G — Suitable for all audiences', 'simple-local-avatars' ),
'PG' => __( 'PG — Possibly offensive, usually for audiences 13 and above', 'simple-local-avatars' ),
'R' => __( 'R — Intended for adult audiences above 17', 'simple-local-avatars' ),
'X' => __( 'X — Even more mature than above', 'simple-local-avatars' ),
) );

I think an E2E test could be most reliable to ensure against the fatal when visiting the front end, with a test that the string Fatal error is not included in the response. I was testing using PHP 8.1.

*/
public function admin_init() {
public function define_avatar_ratings() {
$this->avatar_ratings = array(
'G' => __( 'G — Suitable for all audiences', ),
Copy link
Contributor

Choose a reason for hiding this comment

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

@peterwilsoncc Can we make these strings translatable?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not the keys, they're used by WP for determining the rating of an avatar. The values are the strings displayed to the user.

It's the equivalent of this code in core. https://github.com/WordPress/wordpress-develop/blob/1111d2b9e6d71bb5506e18940518cc96adbbdb97/src/wp-admin/options-discussion.php#L249-L258

Copy link
Contributor

Choose a reason for hiding this comment

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

@peterwilsoncc Can you add a comment? I can see someone can make them translatable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • c75208f Note explaining why the key is note translated
  • 9f8bd3d Added some translator comments duplicated from the same array in Core while I was dealing with i18n issues.

Copy link
Member

@faisal-alvi faisal-alvi left a comment

Choose a reason for hiding this comment

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

@peterwilsoncc thanks for the fix. I have created ticket #188 to handle fatal in a Cypress test. We can work on it separately.

@faisal-alvi faisal-alvi merged commit b09e9fa into develop Jan 16, 2023
@faisal-alvi faisal-alvi deleted the fix/186-avoid-ratings-errors branch January 16, 2023 07:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Plugin crashes after update to version 2.7.2
3 participants