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

Update the 404 template and add a new generic error template #1842

Merged
merged 10 commits into from
May 28, 2024

Conversation

amieiro
Copy link
Member

@amieiro amieiro commented May 8, 2024

Problem

As I explained in #1841, we should improve the error messages. Currently:

  • The die_with_error method shows an empty page with the error text.
  • The die_with_404 method shows a template that can be improved.

image

Fixes #1841.
See WordPress/wporg-gp-translation-events#251.
Fixes WordPress/wporg-gp-translation-events#251.

Solution

In the issue, I proposed to add a template to the die_with_error method and improve the 404 template.

This PR:

  • Updates the 404 template.
  • Updates the die_with_error method to load a new generic template: error.php:
    • If the request is an AJAX request, it maintains the old functionality.
    • If the request is not an AJAX request, it returns a new template: error.php by default.

Old 404 template

image

New 404 template

image

Error with the old approach

image

Error with the new template

image

Testing Instructions

To test this PR:

I tested it here, with the wporg-gp-translation-events plugin, that depends on GlotPress and uses the die_with_error method.

I updated this code:

		try {
			$event_stats = $this->stats_calculator->for_event( $event->id() );
		} catch ( Exception $e ) {
			// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
			error_log( $e );
			$this->die_with_error( esc_html__( 'Failed to calculate event stats', 'gp-translation-events' ) );
		}

With this:

		try {
			$event_stats = $this->stats_calculator->for_event( $event->id() );
			throw new Exception('This is a test');
		} catch ( Exception $e ) {
			// phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
			error_log( $e );
			$this->die_with_error(
				esc_html__( 'Failed to calculate event stats', 'gp-translation-events' ),
				500,
				esc_html__( 'Error', 'gp-translation-events' ),
				'error'
			);
		}

So I forced to fire a new exception, and then I show the new template.

Screenshots or screencast

@amieiro amieiro requested review from akirk and trymebytes May 8, 2024 08:58
@amieiro amieiro marked this pull request as ready for review May 8, 2024 08:58
gp-includes/route.php Outdated Show resolved Hide resolved
@amieiro amieiro requested a review from akirk May 14, 2024 13:59
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Very nice! Could we also take the opportunity to fix a few wrong HTTP status codes?

I found these where I think we should change it to a 404 (note also the typo in the warning message):

gp-includes/routes/translation.php
736:			return $this->die_with_error( 'Translation doesn’t exist!' );
779:			return $this->die_with_error( 'The warning doesn’exist!' );

gp-includes/routes/project.php
470:			return $this->die_with_error( __( 'Project wasn’found', 'glotpress' ) );
521:			return $this->die_with_error( __( 'Project wasn’found', 'glotpress' ) );

gp-templates/error.php Outdated Show resolved Hide resolved
gp-includes/route.php Outdated Show resolved Hide resolved
@amieiro amieiro requested a review from akirk May 27, 2024 21:15
akirk
akirk previously approved these changes May 28, 2024
Copy link
Member

@akirk akirk left a comment

Choose a reason for hiding this comment

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

Just two more little things but otherwise fine for me! Nice work!

gp-includes/routes/translation.php Outdated Show resolved Hide resolved
gp-templates/translations.php Outdated Show resolved Hide resolved
amieiro and others added 2 commits May 28, 2024 06:45
Co-authored-by: Alex Kirk <akirk@users.noreply.github.com>
@amieiro amieiro requested a review from akirk May 28, 2024 04:49
@akirk akirk merged commit b9f364c into GlotPress:develop May 28, 2024
11 checks passed
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.

Improve the error messages Nicer error message page
2 participants