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

Add notice if Xdebug extension is loaded #3198

Merged

Conversation

aaemnnosttv
Copy link
Contributor

@aaemnnosttv aaemnnosttv commented Sep 5, 2019

As discovered in #1915 (comment), xdebug can cause AMP stylesheet parsing to take much longer than normal and potentially crash the site due to maximum execution limit timeout.

This PR adds a notice to inform the user of the potential risk.

I would expect perhaps the wording should maybe change, if so, just let me know!

Screenshot

Screen Shot 2019-09-16 at 08 38 01

@googlebot googlebot added the cla: yes Signed the Google CLA label Sep 5, 2019
amp.php Outdated Show resolved Hide resolved
amp.php Outdated Show resolved Hide resolved
amp.php Outdated Show resolved Hide resolved
amp.php Outdated Show resolved Hide resolved
@westonruter westonruter added this to the v1.3 milestone Sep 5, 2019
aaemnnosttv and others added 4 commits September 5, 2019 23:19
Co-Authored-By: Weston Ruter <westonruter@google.com>
Co-Authored-By: Weston Ruter <westonruter@google.com>
@aaemnnosttv
Copy link
Contributor Author

Updated per your feedback @westonruter.

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

Looks good. I'll wait for a +1 from @schlessera before merging.

amp.php Outdated Show resolved Hide resolved
@westonruter westonruter merged commit 164bcae into ampproject:develop Sep 6, 2019
@aaemnnosttv aaemnnosttv deleted the feature/add-xdebug-notice branch September 6, 2019 15:28
@schlessera schlessera removed their request for review September 7, 2019 12:41
@ghost
Copy link

ghost commented Oct 5, 2019

This should never have been added. If something in your plugin is making websites crash when xdebug is enabled, then surely shouldn’t the issue in your plugin be fixed rather than advising the user to disable the extension?

It’s not the users fault for having xdebug enabled, and most of the time it’s intentionally enabled so there should be no reason to force users to have to see a notice saying they’re doing something wrong.

There’s obviously an issue in the coding of your plugin that needs fixing.

@westonruter
Copy link
Member

@leecollings if it bothers you so much, you can remove it yourself in another plugin with just this:

remove_action( 'admin_notices', '_amp_xdebug_admin_notice' );

@swissspidy
Copy link
Collaborator

I've created a new issue to discuss the placement of the warning: #3499.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants