Make WordPress Core

Opened 9 years ago

Last modified 7 years ago

#33741 reopened enhancement

Remove references to my-hacks.php and the hack_file option

Reported by: bobbingwide's profile bobbingwide Owned by: jorbin's profile jorbin
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.3
Component: Plugins Keywords: has-patch
Focuses: Cc:

Description

According to the documentation https://codex.wordpress.org/Hacking_WordPress my-hacks.php was deprecated in v1.5 and removed in v2.8

So the code in wp_get_active_and_valid_plugins() is no longer necessary
and can be deleted along with mentions of my-hacks and hack_file anywhere else in the code.

Attachments (2)

33741.patch (1.6 KB) - added by bobbingwide 9 years ago.
Remove references to my-hacks.php and hack_file
33741.2.diff (717 bytes) - added by netweb 9 years ago.

Download all attachments as: .zip

Change History (34)

@bobbingwide
9 years ago

Remove references to my-hacks.php and hack_file

#1 @bobbingwide
9 years ago

  • Keywords has-patch added

#2 @SergeyBiryukov
9 years ago

  • Milestone changed from Awaiting Review to 4.4

#3 @swissspidy
9 years ago

  • Keywords dev-feedback added

According to the documentation my-hacks.php was deprecated in v1.5 and removed in v2.8

It's possible that someone's still using this file, but I doubt it. Anyway, some dev feedback on this would be great.

#4 @bobbingwide
9 years ago

If someone is still using the file, and it contains the standard comment for a plugin then wp_get_active_and_valid_plugins() will list it in the list of plugins or mu-plugins regardless of the setting of the hack_file option.

If it doesn't have the comments then it won't get listed in plugins, but will get shown in mu-plugins.

I can't see that as a problem

Last edited 9 years ago by bobbingwide (previous) (diff)

This ticket was mentioned in Slack in #core by sergey. View the logs.


9 years ago

#6 @SergeyBiryukov
9 years ago

  • Keywords commit added

#7 @jorbin
9 years ago

This will need a dev-notes post on make/core when it goes in. It's old and hopefully not used, but we should still caution people since it is a breaking change.

This ticket was mentioned in Slack in #core by jorbin. View the logs.


9 years ago

#9 @jorbin
9 years ago

  • Owner set to jorbin
  • Resolution set to fixed
  • Status changed from new to closed

In 34291:

Remove support for my-hacks.php

For the last 10 years, my-hacks has been deprecated and has been throwing a deprecation notice. For the last six years, you haven't been able to enable my-hacks.php in the admin UI. That should be enough time to give developers notice. Plugins and themes seem like they might have staying power.

Fixes #33741
Props bobbingwide

#10 @Gabriel Reguly
9 years ago

From https://make.wordpress.org/core/2015/09/18/my-hacks-php-no-longer-supported/

What about creating a plugin that will look for my-hacks.php and then load it? :-)

Then it could email the site admin about this situation and prompt about doing the proper plugin.

This way there will be no broken sites.

Just my $0.02.

Cheers,
Gabriel

#11 @nacin
9 years ago

  • Keywords has-patch dev-feedback commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

If an option is going to be removed, it needs to be added to $unusedoptions in schema.php.

Separately: @josephscott previously attempted to remove my-hacks.php. There are almost no CPU cycles gained by removing it, as hack_file is an autoloaded option. It's nice to clean it up, of course, but I'll reveal that he scanned all VaultPress sites for the existence of my-hacks.php and found 6 that still used it. That's a very low number, except that two of them were the sites of WordPress co-founders. So we ended up just dropping it.

I doubt that this will break a site, and I don't care enough to revert this, but we should do a make/core post about it.

#12 follow-up: @jorbin
9 years ago

@nacin a make post was published minutes after this change went in. https://make.wordpress.org/core/2015/09/18/my-hacks-php-no-longer-supported/

I don't think it's the CPU cycles gain that is important here, but instead it is the removal of code and thus resulting simplification of the plugins loading code. my-hacks.php has been an edge case for a number of years and the elimination of that edge case helps the code be clean, lean and mean.

10 years of deprecation seems like enough warning. Do you happen to know the previous ticket when removal was attempted? I couldn't find it when searching through trac. I would also be interested in the VaultPress stats to see if that number has changed. Additionally, I asked @DH-Shredder to look into the stats on his end. Perhaps @mikehansenme would also want to take a look or any of the other hosts as well.

One proposal on the make blog was to attempt to move my-hacks.php to mu-plugins on upgrade. I would be support a patch that did that.

@netweb
9 years ago

#13 in reply to: ↑ 12 @netweb
9 years ago

Replying to nacin:

If an option is going to be removed, it needs to be added to $unusedoptions in schema.php.

This is in 33741.2.diff

Replying to jorbin:

10 years of deprecation seems like enough warning. Do you happen to know the previous ticket when removal was attempted? I couldn't find it when searching through trac.

My trac foo didn't surface anything when I went a looking either...

#14 follow-up: @bobbingwide
9 years ago

  • Keywords has-patch added

I'd just like to point out that I didn't actually read the code properly.
In my mind I'd read the array_unshift line as something completely different.
I thought it was removing the my-hacks plugin from active plugins rather than adding it to the list.

But after due consideration I've decided that my patch to remove the redundant code is still valid.

@nacin

hack_file is only an autoloaded option if it exists.
When it does not exist another query is run after the first one to load the autoload options

1,0.030197,SELECT option_name, option_value FROM wp_options WHERE autoload = 'yes'
2,0.001128,SELECT option_value FROM wp_options WHERE option_name = 'hack_file' LIMIT 1

The TRAC that removed the my-hacks checkbox was #9551.

@nacin @netweb

I can't see the point in adding an options field that should have been unused for 6 years into the $unusedoptions array.

In actual fact, you could actually break some code that was written since WordPress 2.8 and has nothing to do with wp-hacks but inadvertently used the option name 'hack_file'.

In my opinion the $unusedoptions array should be pruned every now and then.
There should at least be some documentation to list the options names that you shouldn't use!



Last edited 9 years ago by bobbingwide (previous) (diff)

#15 in reply to: ↑ 14 ; follow-up: @nacin
8 years ago

Replying to bobbingwide:

hack_file is only an autoloaded option if it exists.
When it does not exist another query is run after the first one to load the autoload options

hack_file continued to be created during install until [34291]. It was always autoloaded.

#16 @nacin
8 years ago

my-hacks.php was mentioned by @josephscott in IRC on August 30, 2013: https://irclogs.wordpress.org/chanlog.php?channel=wordpress-dev&day=2013-08-30&sort=asc#m678600.

He followed up with me over Skype where he shared with me the extremely low usage of the file on VaultPress sites. It never made it to a ticket. In our conversation, moving it to mu-plugins did come up.

#17 in reply to: ↑ 15 @bobbingwide
8 years ago

Replying to nacin:

Replying to bobbingwide:

hack_file is only an autoloaded option if it exists.
When it does not exist another query is run after the first one to load the autoload options

hack_file continued to be created during install until [34291]. It was always autoloaded.

Ah yes. I missed that.

#18 @bobbingwide
8 years ago

So where do we stand on this?
As I said previously. I don't support adding hack_file into the $unusedoptions array since it could break someone else's code.

hack_file=0 my-hacks=N -> OK

hack_file=0 my-hacks=Y -> dormant file as far as core is concerned, but could be loaded some other way
Deleting the file could break someone's code.

hack_file=other my-hacks=N -> Deleting the option could break someone's code.

hack_file=other my-hacks=Y -> How long have they been ignoring the deprecated message?
Deleting the code could break someone's code... but that was the whole point of the deprecated file message.

This ticket was mentioned in Slack in #core by helen. View the logs.


8 years ago

This ticket was mentioned in Slack in #core by jorbin. View the logs.


8 years ago

#22 @jorbin
8 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 35688:

Revert [34291] bringing back my-hacks

Keeping myhacks support is a small price to pay for not breaking people's sites. Even if it is very very very few sites, breaking sites isn't something that should be encouraged. Even with 10 years of deprecation notices.

https://core.trac.wordpress.org/ticket/33741#comment:18 outlines all the ways that the hack_file and my-hacks options can be setup and thus all the ways that the removal of those options could break sites.

Fixes #33741.

#23 @jorbin
8 years ago

  • Resolution changed from fixed to wontfix

I really want to kill my-hacks. I really want to show that WordPress does actually revert or remove deprecated things. Even more so, I don't want to break sites. The commit message above and comment 18 outlines this.

https://www.youtube.com/watch?v=O_4OfD-wmGs

#24 @SergeyBiryukov
8 years ago

  • Milestone 4.4 deleted

#25 @bobbingwide
8 years ago

I don't understand the last few updates in comment:22 and comment:23.

33741.2.diff was the problem that I mentioned in comment:18.

If you don't delete the option then the only sites that get broken are the one's that are supposed to be broken!

Just use the first patch.

Last edited 8 years ago by SergeyBiryukov (previous) (diff)

#26 @bobbingwide
8 years ago

  • Resolution wontfix deleted
  • Status changed from closed to reopened

After due consideration I have re-opened this ticket. If we cannot finally remove code that's been deprecated for 6 years then what chance have we dropping support for PHP 5.2.

The first patch ( 33741.patch ) simply removes the references to my-hacks and hack_file.
It finally implements what it was set out to do.

The second patch ( 33741.2.diff ) which implements "If an option is going to be removed, it needs to be added to $unusedoptions in schema.php" is, IMHO, the flawed logic that could lead to a site being unintentionally broken.

So, as stated before. Don't add hack_file into $unusedoptions.
One day, perhaps when 5.2 support is dropped, the same argument can be applied to all the other "unused options". They're only unused as far as WordPress core is concerned.

#27 @netweb
8 years ago

  • Milestone set to Awaiting Review

#28 @helen
8 years ago

  • Type changed from defect (bug) to enhancement
  • Version changed from trunk to 4.3

I'm not sure I understand why the revert happened either, or why the wontfix. I see the determined reason, but no discussion as to whether not bothering really is the right approach overall.

#29 @nacin
8 years ago

The revert was entirely unrelated to $unusedoptions. It was done because @jorbin was concerned we'd break a few sites without a good reason. The was essentially nixing that we should remove my-hacks.php, and thus the ticket was properly closed as wontfix based on that decision.

My comment about $unusedoptions was *only* that if we were going to go through with [34291], then it was useless to keep around the option in the DB, and it should also be removed. "Deleting the option could break someone's code" is not my concern here — we've removed 'dead' options many times before. This is especially OK to do (in a backwards-compatible context) when the "default" value (in this case "0") evaluates to false (equivalent of it not being in the DB).

#30 @bobbingwide
8 years ago

OK, I still don't understand. This is how I read it.

By adding hack_file to the $unusedoptions we could potentially break someone's code
... and because we think the value's 0, we're happy to do this.

But we're not happy to remove the code that has been telling people for 6 years that their site is going to break one day.

BTW: The title of the enhancement is still "Remove references to my-hacks.php and the hack_file option"
Should I add "ALL"?

#31 @MikeLittle
8 years ago

@bobbingwide Re: https://twitter.com/herb_miller/status/674631292101767168

On one site the file still exists and the option is still set to 1. However, the functions defined there are no longer used in the site's theme as the theme has changed.

Interestingly, if I preview my old theme, I can see that the my-hacks file is loaded but has errors now. I will happily remove it from my installation.

However I agree that no site should be broken by any changes proposed here.

This ticket was mentioned in Slack in #core by swissspidy. View the logs.


7 years ago

Note: See TracTickets for help on using tickets.