ProjectCleverWeb / Semantic-UI-WordPress

This project incorporates Semantic UI into a starter (aka developer) theme for WordPress.
Other
275 stars 59 forks source link

inc/req/once functions are not necessary #29

Closed JJJ closed 9 years ago

JJJ commented 9 years ago

The include/require constructs exist in PHP in a protected and tuned way that a theme living inside of WordPress cannot compete with.

See also: https://core.trac.wordpress.org/ticket/32845

Because each method is unique, and each one extracts all $GLOBALS into local scope, and each is called multiple times, this stampedes out of control very quickly.

Everything should go back to using PHP's built in include & require methods to make this theme usable again.

ProjectCleverWeb commented 9 years ago

NOTE: This applies to a version of this theme that is in active development, so it in some parts applies to functionality and bugs that may not exist in 1.x.x

Include functions/methods beyond PHP's built-in includes

The include/require constructs exist in PHP in a protected and tuned way that a theme living inside of WordPress cannot compete with. … Everything should go back to using PHP's built in include & require methods to make this theme usable again.

Tell that to the guys WordPress, because that is exactly what get_template_part(), get_header(), get_footer(), and get_sidebar() are all there for.

Thing is, without those functions many plugins simply wouldn't work and child themes couldn't exist. So I would say that they greatly improve upon how the PHP includes work.

Extracting $GLOBALS

Because each method is unique, and each one extracts all $GLOBALS into local scope, and each is called multiple times, this stampedes out of control very quickly.

From issue #27:

There are a few places where $GLOBALS is extracted into local scope. This duplicates the data stored in all global variables and causes huge memory spikes resulting in out-of-memory issues.

This functionality is used to help minimize extra code, and make help with some headaches that occur when dealing with globals inside of wordpress templates. (some developers don't know all the intricacies of PHP and how it handles variables)

As far as memory spikes, I will need to see some proof of that. If I remember correctly, PHP uses variables in a strictly copy-on-modify pattern. (cite at ~34:00) That would mean that extract would only be creating variables with references to the original values until the original's or copy's values were changed. So there should only be a minimal spike at worst.

However, this specific part of the include methods is really just a test of viability and need; and I have yet to find an instance where this could be needed for the theme to function properly in its current configuration. So it will probably be removed regardless.

Custom Template Functions

From issue #28:

It's almost impossible to learn how this theme works because it needlessly duplicates WordPress's core template location & loader functions. … I'd like to see this go back to using WordPress's core functions, and let a theme author decide if something more sophisticated is necessary for their project.

Well, the developer can use the WordPress functions. The new functions do not break or replace the existing functions. The simply offer an extended alternative. You can interchange using them without error.

As far as learning the new functions, the're all documented and there is really only 5 new ones that you have to understand (template_part(), template_use_part(), template_header(), template_footer(), and template_sidebar()); and all but 1 are backwards compatible to WordPress functions.

So I must ask, what else I can do to make them easier for you to learn?

JJJ commented 9 years ago

screen shot 2015-09-16 at 4 45 11 pm

what else I can do to make them easier for you to learn?

I don't want to learn them. :) Ideally, WordPress's core functions should be used, because developers can search for them, see how they're being used, and understand immediately what's located where and why. With these new nested inherited methods, that's not possible.

This functionality is used to help minimize extra code, and make help with some headaches that occur when dealing with globals inside of wordpress templates. (some developers don't know all the intricacies of PHP and how it handles variables)

I understand, and your implementation is clever and powerful, but it introduces a bevy of possible collision issues with plugins that aren't expecting for all of these globals to be in the template-loader scope that suddenly are. Plugins like BuddyPress that, for legacy reasons, use globals like $group inevitably get stomped by other plugins, etc...

Tell that to the guys WordPress, because that is exactly what get_template_part(), get_header(), get_footer(), and get_sidebar() are all there for.

I am a WordPress guy, unless something has changed. :) These functions are for locating templates in a stack of registered locations, and passing explicitly allowed global variables to them for legacy reasons. They don't directly replace include & require calls; they're just a simple router & control mechanism for them.

ProjectCleverWeb commented 9 years ago

screen shot 2015-09-16 at 4 45 11 pm

What I meant by proof is some type of performance benchmark, or even just showing that the memory allocated is significantly less when you comment out lines 333, 363, 393, and 423 in the theme class file.

Beyond that, knowing what PHP and WordPress versions would help. And getting the debug report from the browser console would be nice. (login as an admin in WP and the debug report will be there)

Like I said, that specific part will probably be removed; but knowing where there might be hiccups is always a priority.

I don't want to learn them. :) Ideally, WordPress's core functions should be used, because developers can search for them, see how they're being used, and understand immediately what's located where and why. With these new nested inherited methods, that's not possible

The beauty of it is that you don't have to learn. They are extended alternatives, and nothing prevents you from creating templates through the normal channels.

As for searchable documentation, that is a feature planned for the theme. You can see a basic structure for this in the developer notes. It's true that they are excessively nested, but that is to prevent code redundancy across functions.

It might be a pain to read and edit 500 lines code like this, but it's easier than reading and editing 1000 lines where the code is repetitive. Or at least that's my opinion. If more issues about this come into play, I will reduce function/method nesting; but for now it's going to be author's preference.

I understand, and your implementation is clever and powerful, but it introduces a bevy of possible collision issues with plugins that aren't expecting for all of these globals to be in the template-loader scope that suddenly are. Plugins like BuddyPress that, for legacy reasons, use globals like $group inevitably get stomped by other plugins, etc...

I wasn't aware of that. And still, this portion of the include methods will likely be removed.

I am a WordPress guy, unless something has changed. :) These functions are for locating templates in a stack of registered locations, and passing explicitly allowed global variables to them for legacy reasons. They don't directly replace include & require calls; they're just a simple router & control mechanism for them.

The new functions are reflective of their WordPress equivalents, whatever their equivalents are purposed for so are the new functions. The difference only is some added functionality.

Also, using someone else's theme for a starting point is bound to have some extras; and at this point I think it's even fair to say that is what many devs expect.

ProjectCleverWeb commented 9 years ago

In regards to how much memory extract($GLOBALS) uses:

A typical request will call about 15 or so includes via the theme include methods. Which will result in about 1MB of extra RAM used.

In this log, it shows that it used 0.965376MB of RAM across 16 includes.

A typical request for this theme & WordPress uses 11.5MB (WP 4.2.2 on PHP 5.6.8 without any plugins and is extracting globals)


Whilst losing 1MB of RAM isn't a big deal in most cases, having unnecessary variables in a scope where they may not be expected could easily cause problems. And thus, this functionality of the include methods will be removed.