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

Improve Xdebug warning #3499

Closed
swissspidy opened this issue Oct 14, 2019 · 7 comments · Fixed by #4432
Closed

Improve Xdebug warning #3499

swissspidy opened this issue Oct 14, 2019 · 7 comments · Fixed by #4432
Assignees
Labels
Enhancement New feature or improvement of an existing one UX
Milestone

Comments

@swissspidy
Copy link
Collaborator

swissspidy commented Oct 14, 2019

Feature description

As mentioned on #3198 and #1915, there are some issues with the current admin notice that is shown when Xdebug is active. Screenshot:

Screenshot 2019-10-14 at 16 07 09

Mainly, the notice is shown even when it is not actually relevant to the site (i.e. Reader mode is used) or when the user cannot do something about it (i.e. they are not an admin).

I'm seeing that myself as well as I've become numb to that warning and don't even notice it anymore really.

A few thoughts:

  1. Don't display notice when using Reader mode (at least until Send entire templates in Reader/Classic mode through the sanitizers #2202 is done)
  2. Only display notice when user can manage_options
  3. Only display the notice on the AMP plugin settings screen
  4. Make notice dismissible with persistence in user settings (bonus: still always show it on the settings screen)

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

Acceptance criteria

  • Remove notice in favor or Site Health, or at least limit notice to just AMP settings screen.
  • Limit to administrators and/or when WP_DEBUG is enabled?

Implementation brief

QA testing instructions

Demo

Changelog entry

@westonruter
Copy link
Member

I think we should move this to Site Health. Potentially we can also put the warning in the admin bar.

@westonruter
Copy link
Member

We should also consider only showing the warning if WP_DEBUG is enabled.

@westonruter
Copy link
Member

Here's how Site Health is shown on the admin dashboard in a widget:

image

@kienstra
Copy link
Contributor

Hi @westonruter,
Good to see you.

Like you mentioned, what do you think about:

  1. Removing the warning entirely as it exists now
  2. Display the warning in Site Health if Xdebug is enabled and WP_DEBUG is true

xdebug-here

@westonruter
Copy link
Member

2. Display the warning in Site Health if Xdebug is enabled and WP_DEBUG is true

If it is true or if it is false? I should think that the warning is most relevant when WP_DEBUG is false as this can indicate the site is running in production where Xdebug certainly should not be active.

@westonruter westonruter added this to the v1.5.1 milestone Mar 20, 2020
@kienstra
Copy link
Contributor

Ah, good point. If Xdebug enabled and WP_DEBUG is false.

@kienstra kienstra self-assigned this Mar 24, 2020
@westonruter westonruter modified the milestones: v1.5.1, v1.5 Mar 24, 2020
@kienstra
Copy link
Contributor

Testing Steps

  1. Go to Site Health
  2. Click 'Passed Tests'
  3. Expected: this appears:

expected-site-health

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or improvement of an existing one UX
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants