WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#8568 closed defect (bug) (wontfix)

Plugin activation hook should not be run when scraping errors.

Reported by: sambauers Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.7
Component: Plugins Keywords: has-patch tested dev-feedback
Focuses: Cc:

Description

The changes in [9352] introduced the running of the plugin activation hook when the plugin is only being tested for PHP errors.

The action should probably not be done in that case.

Attachments (1)

8568.diff (520 bytes) - added by Denis-de-Bernardy 5 years ago.

Download all attachments as: .zip

Change History (14)

comment:1 DD325 years ago

The idea was to allow for fatal errors within the activation hook to be displayed, Any fatal error which occurs during the activation hook on activation will cause the plugin not to be activated, and then display an empty scrape page (Since the fatal error inside the activation hook is never hit)

It also allows for plugins to test conditions (such as wordpress version) on their activation hooks, and to prevent the activation if a bad-condition is met.

comment:2 DD325 years ago

  • Keywords reporter-feedback added

In addition to the above comment, The Plugin scrape page will only be loaded after activation has been attempted once, It should fail exactly the same way upon a 2nd attempt.

Sam: Do you have any arguments to this approach? Or for that matter, Anyone?

comment:3 Denis-de-Bernardy5 years ago

I tend to agree with DD32. Would it not make sense to run the deactivate hook as well when testing if the plugin works? After all, it would be wise to do that as well, no? Plus, it would make it so that the plugin's settings are back to their deactivated state the next time the activate hook is fired.

D.

Denis-de-Bernardy5 years ago

comment:4 Denis-de-Bernardy5 years ago

  • Keywords has-patch dev-feedback added

patch adds a call to deactivate plugin.

comment:5 DD325 years ago

absolutely no point running the deactivation hook IMO, since the plugin was never "active" mind you.. i guess it cant hurt.

however, it should -never- reach a point where the deactivation code is run, the plugin should've failed with a fatal error, or the activation function should've printed a error reason and died..

comment:6 Denis-de-Bernardy5 years ago

I disagree.

Suppose a plugin creates a table or adds a field on activate, and removes it on deactivate. Then the wp error checking code will run activate, and activate will run once again upon getting "truly" activated. For this reason, if activate gets run, deactivate should get run as well.

Then, picture a plugin with a fatal error in the deactivation code. clearly that area of the code should be tested as well.

comment:7 hakre5 years ago

It is a recommended practise to place Upgrade, Cleanup, Install-Checks etc. _in_ the Activation Hook 1.

The Request in Question is 'error_scrape'. I can not find a clear description what this means and for what the code is intended for.

As long as it remains unclear wether or not the plugin shouldn't be actived through 'error_scrape', the code should remain as it is and an additional Hook should not be called.

Anyway, I adopt the criticism in this patch partly: It is simply unclear what should happen here. Is the Plugin Activated anyway afterwards or is this a Test _only_?

What about creating an additional Testdrive Process that is run only on Activation?

comment:8 DD325 years ago

The Request in Question is 'error_scrape'. I can not find a clear description what this means and for what the code is intended for.

It if for after a failed activation, To display the errors produced to the web browser.

Anyway, I adopt the criticism in this patch partly: It is simply unclear what should happen here. Is the Plugin Activated anyway afterwards or is this a Test _only_?

It is a re-creation of a activation failure, with the intent to display the error to the browser.

What about creating an additional Testdrive Process that is run only on Activation?

This happens, If upon activation, A error occurs, Then the plugin will not be activated, Instead, It'll direct back to the plugins page, an an iframe with error_scrape will load up.

comment:9 Denis-de-Bernardy5 years ago

  • Keywords tested added; reporter-feedback removed

comment:10 Denis-de-Bernardy5 years ago

  • Milestone changed from 2.7.2 to 2.8

comment:11 ryan5 years ago

  • Component changed from Administration to Plugins
  • Owner anonymous deleted

comment:12 follow-up: hakre5 years ago

I - as a plugin developer - am quite confident with the current implementation. It ensures that errors created by plugin activation are covered. This is usefull.

Calling the Deactivation Hook must not be necessary here because the code in question is only executed if the Activation Hook fails. So I assume it will never be executed? Or will it?

comment:13 in reply to: ↑ 12 Denis-de-Bernardy5 years ago

  • Milestone 2.8 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

Replying to hakre:

I - as a plugin developer - am quite confident with the current implementation. It ensures that errors created by plugin activation are covered. This is usefull.

Not really. It won't cover a variety of other potential errors that could be caused by a plugin's internal functionality. The current implementation will merely check for valid php syntax. It's not as if it ran every last hook in $wp_filter with the correct set of arguments to check if all goes well indeed.

Calling the Deactivation Hook must not be necessary here because the code in question is only executed if the Activation Hook fails. So I assume it will never be executed? Or will it?

To me, whichever works, since I never clean up the options I introduce -- in case the user reactivates the plugin later on. I think the issue Sam was concerned about is more along the lines of: on activate, create a db table (without checking if it exists). you can then get an actual error.

Anyway, let's just close this as wontfix, as there obviously isn't much traction. :-)

Note: See TracTickets for help on using tickets.