Great module. How about adding X-Autoload module support?
What do you think?

Comments

zengenuity’s picture

I considered this when I first wrote the module, but I'm not sure that it will really make a lot of positive difference. We're generating two classes per bundle + the base entity classes, so even complex sites should have like 50 classes or so. How does that compare to the thousands of PHP files that such a site would have? I haven't tested it, but my initial thought is that it would be fairly insignificant. I could be wrong on that. Do you have any metrics about how much additional overhead adding these classes creates? Versus the overhead of X-Autoload itself? It's not something I've tested in detail, so if you have experience you can share that would be helpful. The current setup has a simplicity advantage of not having to worry about loading at all, which for D7, I think makes sense.

For D8, the system will support the built-in autoloading and namespace features for sure.

yogaf’s picture

Nope, I don't have metrics to support this.
But sure there are people who think using x-autoload is more efficient and have better DX.(https://www.deeson.co.uk/labs/extending-entitydrupalwrapper#comment-1721...)

If there are no objections from your side, I'll try to add support for X-autoload.

zengenuity’s picture

Can we do a minimal amount to get it running and see what the performance impact is? I'm no opposed to autoloading generally, but here are some concerns I have:

1. What changes will be required to the folder structure of the classes to make this work? How can we handle existing installs? (not many of them currently, but at a minimum we need to know the manual steps to convert setups)

2. Will x-autoload be a requirement? I am opposed to that unless it can be shown to be a lot better. It's not a standard practice for D7, so forcing people to use x-autoload for the few Wrappers Delight classes seems unnecessary. The performance comment he makes in that post about reading from the DB is probably valid, but I just checked the registry table on a fairly simple site of mine, and it's got 1124 classes listed in that table already. (679 if you take out the test classes) I'm skeptical about the impact of a few more.

3. I think his DX comment is true but minor. Devs clear cache all the time when adding files (for templates, etc), and the Wrappers Delight Drush command actually clears the cache for you.

So, overall, since it's not standard practice for D7, I'm not committed to the idea of adding autoloading unless we can show that it's significantly better for performance. A minimal patch that gets it working for a clean install should be enough to test that.

yogaf’s picture

Oh, I guess I should try to be clearer next time :)
When I'm talking about adding support for X-Autoload I only meant as optional feature.
Something like "drush wrap node --x-autoload" will create x-autoload structure while "drush wrap node" without the --x-autoload parameter still working as usual.

zengenuity’s picture

As long as it's optional, I think it's okay. Let's see how it works.

We should probably have a configuration variable that you can set in settings.php to default the --x-autoload option on. Users who choose that option are going to always want it on, so it will be tedious for them to type it every time.