ClassicPress / ClassicPress-v1

A copy of ClassicPress v1.x.
1 stars 1 forks source link

Remove capital_P_dangit() function #196

Closed wolffe closed 6 years ago

wolffe commented 6 years ago

Remove capital_P_dangit() function.

Along with other filters, this function can be expensive CPU-wise if there's enough traffic/activity and content.

Has patch(es) here:

https://github.com/ClassicPress/ClassicPress/pull/176 https://github.com/ClassicPress/ClassicPress/pull/177 https://github.com/ClassicPress/ClassicPress/pull/178 https://github.com/ClassicPress/ClassicPress/pull/179

senlin commented 6 years ago

Great issue and PR, thank you @wolffe

Usually things have to go through petitions first, but since you already supplied the PR, we will discuss this in the committee.

Thanks again!

nylen commented 6 years ago

I was wondering when this would come up.

I'd like to see more information about the actual problems this function causes, and even after that, I'm not sure this is a good candidate to do in ClassicPress v1.

In the meantime I'm going to close these PRs. If we change something here, it needs to be all done in a single PR.

wolffe commented 6 years ago

It's my first patch. I have a plugin, called Lighthouse, which disables lots of default features, and I'd like to natively remove them, instead of using hooks, filters and actions.

nylen commented 6 years ago

I'd like to natively remove [lots of default features], instead of using hooks, filters and actions.

Generally speaking, the problem with this is that if we remove a feature, it's gone for everyone, including the people who do actually make use of it.

Several of our most-voted petitions are petitions to remove various WordPress features. First let's take XML-RPC as an example.

If we remove XML-RPC from ClassicPress, then this will mean that no one can use Jetpack, or the official WordPress mobile apps with a ClassicPress site. This will break many other plugins and possibly even some themes.

Now back to the specific example of capital_P_dangit... it exists because there is benefit in making sure our brand always appears as ClassicPress instead of Classicpress. In fact, I'd consider leaving the capital_P_dangit in place for both WordPress and ClassicPress for that reason.

My understanding is that the initial implementation of capital_P_dangit was buggy, but it rarely causes actual problems anymore, whether with performance or unwanted content replacements. It's enabled on many large-scale WP sites, and this so far down the list of their performance concerns that it doesn't register as a high-priority issue.

If we are to remove capital_P_dangit for everyone, I'd like to see some evidence of actual problems (performance timings, etc), and evidence that the cost and frequency of these problems outweigh the benefit of ensuring that our name is written consistently.

Otherwise, I'd suggest that we postpone this change until v2, and consider moving capital_P_dangit (yes, even this) into a core plugin that is enabled by default.

In the meantime, it is quite easy for individual ClassicPress sites to disable this functionality:

remove_filter( 'the_content', 'capital_P_dangit', 11 );
remove_filter( 'the_title', 'capital_P_dangit', 11 );
remove_filter( 'wp_title', 'capital_P_dangit', 11 );
remove_filter( 'comment_text', 'capital_P_dangit', 31 );
remove_filter( 'widget_text_content', 'capital_P_dangit', 11 );
Mte90 commented 6 years ago

And anyway we can do that for 2.0.0 in case with a petition.

nylen commented 6 years ago

Yep, that is what I think we should do here: put this through a petition like other major changes.

mikeschinkel commented 6 years ago

It's my first patch. I have a plugin, called Lighthouse, which disables lots of default features, and I'd like to natively remove them, instead of using hooks, filters and actions.

I wonder if he means more of an options, not decisions approach?

By that I don't necessarily mean to add a settings page in the admin with 1000 settings, but instead maybe a long list of named settings that can simply be specified in the config file rather than needing to write a hook and decide where to install it, i.e. the theme's functions.php, which may be littered with other code already, or the addition of yet another little plugin?

Note that to go this route would ideally mean coming up with a better approach for settings besides yet another immutably define()d constant (that makes setup for automated testing harder.)

wolffe commented 6 years ago

I also mean options, not decisions, but I also mean complete removal of some of these features.

mikeschinkel commented 6 years ago

@wolffe Wouldn't moving to a core plugin be better for everyone rather than complete removal? That way, if someone actually wants/needs something, they can still use it, right?

(Though capitalPDangit() is not something I would argue to keep. :-))

wolffe commented 6 years ago

Of course, just remove them from the core. I don't care where they go.

wolffe commented 6 years ago

We could have a "companion" plugin, containing all useless WordPress code, called CP WordPress Companion. All the functions, such as the one above, could go there, for people still using it, and actually measuring its usage.

We could have separate sections in this "companion" plugin, for content parsing, for comments, for the customizer, for head options, for RSS, generally for everything that goes out of the core.

mikeschinkel commented 6 years ago

Personally I would rather see core plugins for each feature, so I could mix and match only the features I wanted for a specific project.

wolffe commented 6 years ago

Yeah, well that's the purpose of ClassicPress, I hope. Strip WordPress to its usable core and (optionally) move all useless, blogging-related, emoji-enhanced, deprecated, obsolete code to separate plugins.

BenOvermyer commented 6 years ago

Rather than making this a matter of opinion, wouldn't it be better to offer hard benchmark numbers that prove removing feature X improves performance?

wolffe commented 6 years ago

Rather than making this a matter of opinion, wouldn't it be better to offer hard benchmark numbers that prove removing feature X improves performance?

I agree, although the capital_P_dangit() is common sense. It shouldn't be in core. And there's more, and opinion plays a part in this. You'll never have 100% consensus.

senlin commented 6 years ago

Can we close this or should it remain open?

nylen commented 6 years ago

It's a valid suggestion, but the right place for it for now is on the petitions site. I've created one, so I'm closing this issue: https://petitions.classicpress.net/posts/107/remove-capital_p_dangit-content-filter

ClassyBot commented 3 years ago

This issue has been mentioned on ClassicPress Forums. There might be relevant details there:

https://forums.classicpress.net/t/remove-capital-p-dangit-content-filter/2847/1