Add an autoload.php in the repo root to control the autoloader of front controllers

Moved.

ahaha I thought moving would be sufficient so I added the Quick fix tag :)

This allows people to use a composer.json even more than intended. On top of that it removes a couple of hacks we have currently in Drupal.

Sadly authorize.php itself is quite big, but I fear that we can remove it anyway, as nobody cares about the install module functionality.

1. +++ b/.htaccess
   @@ -129,7 +123,7 @@ DirectoryIndex index.php index.html index.htm
   +  # Allow access to PHP files in /core (like Statistic module's statistics.php):
   

   Statistics.

2. +++ b/.htaccess
   rename from core/authorize.php
   rename to authorize.php
   

   Shouldn't we move most of authorize.php to a new file in /core and include it from the front controller, we already have an issue to put most of index.php back into /core.

3. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -833,15 +833,7 @@ protected function initializeRequestGlobals(Request $request) {
   -        // Remove "core" directory if present, allowing install.php,
   

   Yay.

Great work!

Rerolled against changes from #2364157: Replace most existing _url calls with Url objects.

.

Re-rolled

I was going to reset RTBC as per #14 but I see the file sizes differ somewhat. Was it a straight reroll?

@hussainweb it is the same patch (I verified), just that #17uses renames for core/install.php -> install.php and core/rebuild.php -> rebuild.php instead of explicit "deleted file mode" and "new file mode"

I think we need a new 8.0.x to 8.0.x change record here and a search to find the original CR for moving them so we can unpublish that one.

Here is the old change record: https://www.drupal.org/node/1327978
Here is a draft change record: https://www.drupal.org/node/2420145

I have not unpublished the old change record. I am guessing we should do that on RTBC or when it gets committed, correct?

Come to think of it, I don't think we can unpublish the old change record as it contains information for other files as well which still holds true. I guess we just have to edit that change record to add these files to the list of the files that remain in the old location.

yes. i updated existing change notice

reroll

I just checked the patch in #26 and see that a file was missing. I just rerolled from #17 again to keep things simple.

RTBC as per #24.

Fixing the failure.

I don't think this will work for statistics.php and run-tests.sh

I have an idea.

Totally new approach so no interdiff.

diff --git a/autoload.php b/autoload.php

I think we should add a rule in .htaccess that user can't access this directly.

@jibran good point.

@tstoeckler, @almaudoh, @ParisLiakos - what are you thoughts on the recent patches - shall I update the issue summary to reflect the change of direction?

I was thinking about this approach earlier, but it was in a different context. My main concern then was that there is a new file to be loaded in the request pipeline. I guess it does not make a lot of difference.

I think it might make some sense to have a separate autoloader (which we can readily use in another file, such as drushrc.php). But still, I wish we could do away with authorize.php, update.php, etc... in core directory.

Yes, i agree with #40.
It would be nice to move those files to the root to get rid of hacks (that only work for Apache anyway), but the approach solves the problem here, so from my side its RTBC.
Would like to leave it to tstoeckler though.

@alexpott: I have to say the new approach is simpler, but like the others I have to ask: would there be some other situation in future where we would want to change install.php, authorize.php, etc? These are front controllers in their own right.

The patch solves the current issue though. Let's hear from @tstoeckler.

I totally not against the moving the front controllers. But it does not solve the composer issue - see run-tests.sh and statistics.php (or any other contrib module that wanted to do something similar).

Do we need to update web.config for the same rule?

re #47 afaics we don't have any equivalent rule in web.config.

Back to rtbc then

Hm, I need a little bit of help understanding this. This patch basically makes the path:

autoload.php -> core/vendor/autoload.php -> core/vendor/composer/autoload_real.php

That's a lot of layers of indirection, and not something I've seen done in any other project.

And from the issue summary:

By placing it into the root of the repository we make it legitimately editable by people using Composer to manage their sites, so that they can simply alter the default autoload.php to include the autoloader they want.

Why is hacking a core file "legitimately editable"? I guess because people using your Composer workflow are only downloading the /core part of Drupal, not the / part of Drupal that contains index.php etc?

Basically, I think the header of the authorize.php file needs to either contain or point to some specific docs explaining all of this a bit more. As it stands, from an "outside" point of view, it's pretty confusing.

diff --git a/.htaccess b/.htaccess
index 7db5d3e..479f923 100644
--- a/.htaccess
+++ b/.htaccess
@@ -138,7 +138,8 @@ DirectoryIndex index.php index.html index.htm
   # custom modules or to run another PHP application in the same directory.
   RewriteCond %{REQUEST_URI} !/core/modules/statistics/statistics.php$
   # Deny access to any other PHP files that do not match the rules above.
-  RewriteRule "^.+/.*\.php$" - [F]
+  # Specifically, disallow autoload.php from being served directly.
+  RewriteRule "^(.+/.*|autoload)\.php$" - [F]
 
   # Rules to correctly serve gzip compressed CSS and JS files.
   # Requires both mod_rewrite and mod_headers to be enabled.

What's the rationale for this change? If we're disallowing all .php files, that should include autoload.php, no?

.htaccess is editable per project and included in our repo - I would argue that editing anything outside /core is not hacking core. In fact this issue once upon a time moved all of the front controllers into the root directory so you could edit them.

So what makes Drupal a special flower.

1. We use composer (kinda) but not at the root because we also want to support the modules on d.o. not using composer.
2. We have multiple front controllers all over the shop and we want to allow modules to supply their own - see statistics
3. We want to allow people to manage their projects using composer if they wish - this means they have to change the autoloader to use a vendor not in ./core

Joomla split their repo to do this and created Joomla framework see http://magazine.joomla.org/issues/issue-aug-2013/item/1450-getting-start...

We never did the necessary work wrt to how we package Drupal or organise the repositories.

Basically, I think the header of the authorize.php file needs to either contain or point to some specific docs explaining all of this a bit more.

Do you mean autoload.php? I guess you do.

What's the rationale for this change? If we're disallowing all .php files, that should include autoload.php, no?

That rule only blocks access to .php files in a subdirectory of the root folder. Autoload.php is, by design, in the root folder.

Hmm. We need a composer meta. I just duplicated this here: #2380389: Use a single vendor directory in the root

The main difference being that we should check for the existence of DRUPAL_ROOT/vendor/autoload.php before including core/vendor/autoload.php. That way autoloading works immediately after you've said composer install, without modifying autoload.php.

The main thing standing in the way of a reasonable Composer implementation is the attitude that changing composer.json is 'hacking core.' It isn't. It's managing your project. If we were to overcome that, we wouldn't have discussions of whether autoload.php were legitimate for the root directory, because we would instead just do the normal thing and let composer put the autoload.php file where it should be and then not care at all. :-)

Because we are working around this expectation, we end up with an autoloading front controller and the possibility of having the user manage two vendor directories when they say composer install.

I'm giving a Composer talk at SANDcamp this coming weekend. Also will be at MidCamp if anyone wants a BoF or other discussion about Composer. And I guess I need to submit the talk again for DrupalCamp LA because no one seems to get how odd it is that we have this constraint on a modern PHP project.

#2432893: Make Compser DrupalWTF Go Away

Putting a file_existsin index.php or the new autoload.php added by this patch is not a great idea - that will an unnecessary file operation to every request.

I understand the concern of a file operation, but we're already loading root/autoload.php and then subsequently root/core/vendor/autoload.php. Which then loads a bunch of files itself, because it is *also* a façade for some autogenerated class loading code. So we're up to a minimum of three file loads already without checking if a file exists. Furthermore, file_exists() caches its result, so we only do it once.

And again: The big concern seems to be that people will 'hack core' so let's just make one less file for them to need to 'hack' in order to make the top-level composer.json work.

Or, of course, just ditch this hobo contraption and just use composer like a normal PHP project. :-)

I'm ok with it either way. It's nice if it works out of the box, but most people are not going to use composer in the first place and it seems silly to enforce a file_exists() on a file that is not going to exist for 95%* of users.

Then again, we perform a file operation every time module_load_include() is run (or whatever the D8 equivalent is).

As for me, I'll be modifying autoload.php anyways, because I prefer for my composer.json and my vendor/ to be outside the webroot completely.

I also think most enterprise users are not going to mind modifying a single file, because as you've already stated, you're not "hacking core" if you modify .htaccess or composer.json (or anything outside of core/ for that matter).

Also, your snide comment that Drupal is not like every other project, simply is not true:
https://www.drupal.org/node/2432893#comment-9654365
but let's leave that discussion for the other thread.

* I admit that I have no idea how many people will use Composer, but I feel it will be a minority.

Most enterprise clients are going to modify that file for performance reasons anyways (i.e. to remove the file_exists).

The only thing I'm concerned about, is that technically now we have somewhat of a circular dependency, drupal/drupal requires drupal/core and then drupal/core requires at least an autoload.php file.

However, I admit that that does not mean you must use drupal/drupal you could use any package, but the package would have to implement an autoload.php in ../core (whereever that is).

I think that's an acceptable loss to get us to the point where we're only loading from a single autoload file. In an ideal world, all of the "entry points" would be in drupal/drupal (which would remove the circular dependency). This is what they've done in Symfony and Symfony Standard. All of the entry points are in the latter, which means the autoload path can be changed on a project level and the former does not give a rip as long as it's loaded.

Since I can't see that happening in the 8.x cycle, I think what we have is as good as it's going to get.

As for me, I'll be modifying autoload.php anyways, because I prefer for my composer.json and my vendor/ to be outside the webroot completely.

That is a worthwhile goal. In Symfonyland we have web/ which is where the front controller lives, and no one mistakes the root for web/. How did it happen in Symfonyland? A technical need was understood to exist, and then engineers decided it was a good idea to address this technical need with a technical solution.

But because we live in Drupalland, where things are a certain way for cultural and political reasons rather than technical ones, we have issues like this one, where we have to argue whether to do file_exists() in a file that acts as a facade for another facade file in order to meet our cultural needs about 'hacking core.' Not 'hacking core' is a cultural need that arose from a technical one, but is now causing us to introduce DrupalWTF all over the place, because it is a cultural need at odds with a technical solution.

As far as how many will use composer to install Drupal 8 in it's current state: NONE, because it doesn't work, and when it eventually does, the instructions will say: "Use the root composer.json file in order to manage your own dependencies. But also change the autoload.php file so it points to the autoload.php file that is managed by composer. We know this is different from the way you're used to using composer, but trust us, it's better because you're not 'hacking core.'"

You will find, in the future, that if we *can* have a painless Drupal installation through composer, that will become the preferred way of installing it, more ubiquitous than git or even the tarball.

I hear what you are saying, as I've said, I don't think modifying index.php is hacking core at all since it's on the "project level" (i.e. not in the core directory).

However, that's not the only entry point to Drupal, and unless we are willing to move all of the entry points into the project level, I think this is the next best solution.

And I for one will always install Drupal with composer, because it is painless. You can do something like:

composer create-project drupal/drupal drupal-site/ 8.0.*@beta

and with that Drupal is ready to go. If I want to add custom dependencies (only a developer is going to do that anyways), I know from the Drupal documentation (and the readme in composer.json) that there is an extra step I need to do. I think that's perfectly acceptable given where we are in the 8.0 cycle. It's not ideal, I agree 100%, but unless we're willing to move all the entry points, and the vendor directory, that's where we are at.

@Mile23

But because we live in Drupalland, where things are a certain way for cultural and political reasons rather than technical ones

The technical reason why don't have a /web directory is because we target shared hosting as a platform to run Drupal on. @Mile23, I find what you've written to be insulting to other core developers. Also, somewhat ironically, by what you've written you've made a technical issue with a technical solution, political by comparing Drupalland to Symfonyland.

btw the point about the file_exists() is also technical and not political - we try to minimise all disk access before page cache is checked. Adding a file exists here is most certainly before the page is cached. Also technically this one would not be cached, quoting the PHP manual:

You should also note that PHP doesn't cache information about non-existent files. So, if you call file_exists() on a file that doesn't exist, it will return FALSE until you create the file.

I understand the concern of a file operation, but we're already loading root/autoload.php and then subsequently root/core/vendor/autoload.php. Which then loads a bunch of files itself, because it is *also* a façade for some autogenerated class loading code. So we're up to a minimum of three file loads already without checking if a file exists. Furthermore, file_exists() caches its result, so we only do it once.

Loading a file via require/include is completely different from file_exists() on a non-existing file. This has been pointed out many times before, such as #340723-108: Make modules and installation profiles only require .info.yml files. Stracing apache is a straightforward way to see what the difference is.

Or, of course, just ditch this hobo contraption and just use composer like a normal PHP project. :-)

"Normal PHP projects" don't have constraints such as being able to install from scratch by unpacking a tarball on some hosting then visiting install.php (with few or zero additional steps depending on the hosting). Integrating composer (and vendor libraries) needs to be done with an understanding of the existing feature set that Drupal has and its use-cases. Sometimes that means re-examining the use-cases/features, sometimes it means changing things upstream (like PSR-4 and Composer support for it), sometimes it means compromises or Drupal-specific adaptations.

Also on "Don't hack core" - that's a dogma, but like many technical dogmas it evolved from sound technical concerns. In my day job we still end up auditing client sites with undocumented hacks to core and/or contrib modules which are completely unnecessary, and I'm always thankful when I go to upgrade a module and can see at a glance which upstream patches are applied to it.

Most enterprise clients are going to modify that file for performance reasons anyways (i.e. to remove the file_exists).

Generally we expect large sites to swap out things like cache backends, but we don't expect them to manually undo bad performance choices made by core. Also this assumes that a medium-sized site running on a VPS (or cloud hosting with a slow file system) isn't going to have a problem with disk access on every request (including internal page cache hits).

Thanks for the docs and the explanation on the autoload.php rule in .htaccess (oops, duh). My concerns are addressed with the patch itself, but not sure I'm the most qualified to commit it, so I'd rather defer to catch on this (sounds like he's +1).

Mile23, you're definitely not winning any friends nor arguments here with your attitude, so I'd recommend recalculating a different strategy.

This isn't a tantrum over file_exists().

I'm saying that there's a bigger problem with Drupal's Composer stuff. And also kindasorta with the Drupal dev process.

I'm not trying to be insulting. It's just frustrating.

The current patch creates a need to hack core by design, something I was told is very important not to do by @webchick in IRC. This is why I went along with the dual composer.json thing to begin with. It's the best workaround for an absolute technical need to not hack core. It's also why I didn't start on other modifications and decided to instead focus on the grunt work of stuff like this: #1800046: [META] Add missing type hinting to core docblocks, fix Drupal.Commenting.FunctionComment.Missing*

However, now we find ourselves in the position of asking users to intentionally hack core for some use cases, in order to support the brittle solution that is a workaround for the principle of not hacking core. Do you see the irony?

This is different from suggesting the user modify .htaccess or the example gitignore, because Drupal won't be able to autoload its classes unless you hack core in these cases.

If users are now allowed to do whatever they want in DRUPAL_ROOT, then life gets a lot easier for people who want to design a system to let contrib specify dependencies through Composer. It also means that our workaround 'working example' composer.json system is kind of silly.

So can I go ahead and work on things that 'hack core' now, or not? If not hacking core is an absolute technical need, then no, I can't. If not hacking core is really a gray area, then why was I told it wasn't?

(Answer: It's not really a technical need, as proved by #66 on and the RTBC. And that's what I'm talking about when I say 'cultural or political need...' Just that the need is mitigated by other factors which no one will tell you, but you are nevertheless expected to know.)

So can someone with the authority to do so please write down the conditions under which a user is allowed to hack core, so that Drupal devs can then refer to this specification when trying to understand how to solve a complex problem? Thanks.

The goal (at least for me) in #1975220: Allow a Composer user to manage Drupal, modules, and PHP dependencies with a custom root composer.json was to move Drupal's code into /core, which then frees up composer.json to be modified by the user.

I think in /core nothing should be modified (I'm not even going to keep that folder in my repo's since composer will load it for me anyways). Anything outside of that folder, well, is more of a grey area. I wouldn't willy-nilly hack anything outside of /core, but I don't think there's a strict rule against it. Anything that should never be modified by the user should really be in core/.

The same thing is true for any other library. Composer even recommends leaving the vendor directory out of your repo.
https://getcomposer.org/doc/faqs/should-i-commit-the-dependencies-in-my-...

Because of this, if you modify anything in vendor/ you're hacking that library. I look at Drupal core the same way.

As far as the "working example sillyness" you speak of, as I've said before, if you execute this command:

composer create-project drupal/drupal drupal-site/ 8.0.*@beta

it works, right now without modifying anything at all. If you were to delete the <root>/composer.json it would stop working. After you run this command, you're free to modify the <root>/composer.json however you'd like, you'll also need to follow the instructions in the file and modify autoload.php.

I also see now that not having file_exists is the way to go, so I approve this change. :)

@alexpott: Also, somewhat ironically, by what you've written you've made a technical issue with a technical solution, political by comparing Drupalland to Symfonyland.

That was in answer to @davidwbarrat's reference to a comment he left in another thread. See #60 where he refers to this comment: https://www.drupal.org/node/2432893#comment-9654365

Symfony was completely refactored in version 2 for something like the use cases we are discussing here (among others, of course). It's fair to say that David's comparison isn't completely wrong, but it's not at all right in the context he's making it, because the two communities' development attitudes are not at all the same.

Here's a non-rhetorical question: What engineering principles are we using to refactor Drupal for the use case of someone who wants to specify a vendor directory outside of core?

It's non-rhetorical because we have a patch. The patch adds an autoload.php file which acts as front controller for another front controller for code autogenerated by Composer. It also leaves core/vendor intact which may or may not be an issue... The reasons for supporting this use case are unclear on whether this matters.

So we have someone (David) who wants to move their vendor directory to a non-standard location. We have no idea why. We have a patch that violates some rule about hacking core, for some reason that is deemed justified by maintainers. We have a solution that leaves vendor/ in place within core for some reason that might be related to the rule about hacking core, but which also might be a problem for David. This patch is RTBCd by the person who asked for it who is not a maintainer.

Where's the engineering?

What is the basis for supporting this use case by asking people to hack core?

The Symfony solution was to separate everything into decoupled packages. Ours is to... what? Decide it's OK to tell users to hack core.

I'm not saying we shouldn't do it, I'm saying that if David wants to bring up that other comparison I'll talk about it, and I'm also here to correct you: It is not a political comparison. It is a realistic comparison of differing development processes. The unfortunate part is that if I talk about it, there is political fallout within the Drupal community *for me.*

@davidwbarrat: I think in /core nothing should be modified

We are in agreement.

Anything outside of that folder, well, is more of a grey area. I wouldn't willy-nilly hack anything outside of /core, but I don't think there's a strict rule against it.

Historically, there has been a strict rule against just that, communicated to me by a maintainer in regard to Drupal's Composer stuff. Apparently it's not a strict rule any more, as of now. For some reason.

I'm totally OK with that. It's something I want.

+1 on the 'needs change record' so we can reflect to the rest of the world that DRUPAL_ROOT is now userland.

I just want to point out that Symfony 2.0 was released on July 28, 2011
http://symfony.com/blog/symfony-2-0

The initial release of Composer wasn't until March 1st 2012
https://github.com/composer/composer/blob/master/CHANGELOG.md#100-alpha1...

It's a little nonsensical to think that Symfony was refactored to use Composer. I actually remember some crazy depency management system that had .deps files etc. That's all been destroyed (in the 2.x lifecycle) and they've embraced Composer. I don't think that happened overnight at all. I don't think PSR-0 happened overnight either.

My point is only that we don't have to refactor all of Drupal to use Composer. We can certainly make it better (like moving the /vendor directory up a level or moving it out of the repository, etc.), but that doesn't have to happen today. We can take this in increments and I think that's perfectly acceptable.

I also want to point out that there is an effort to decouple components from core:
https://github.com/drupal/drupal/blob/8.0.x/core/lib/Drupal/Component/RE...

Each of the components (https://github.com/drupal/drupal/tree/8.0.x/core/lib/Drupal/Component) are going to need read-only subtree splits (I haven't created the issue yet), much in the same way Symfony has them (https://github.com/symfony).

I only point this out to say we're on the right track, and it's not an overnight job by any means.

See also: https://www.drupal.org/node/1818628#comment-9672571

Rerolling to keep this issue alive.

We added the /core directory so that modifying files below that level doesn't end up being 'hacking core' - the idea being that by removing all meaningful code from those files they won't change during the entire 8.x cycle (short of an absolute emergency we couldn't anticipate) so there'll never be conflicts to resolve.

This patch is consistent with that (as well as enabling better composer setups) so committed/pushed to 8.0.x, thanks!

catch,

Should the change record be published?

The change record about requiring users to change files in DRUPAL_ROOT to enable normal functioning? I don't see that one.

#82,

It's been in the docs for a while, I'll update it later to reflect this change.
https://www.drupal.org/node/2404989

That's not a change notice.

Updated to reflect the changes.
https://www.drupal.org/node/2404989

if you'd like a change notice, you're more than welcome to write one. :)

This issue has caused a regression #2464055: Installation Failure on case insensitive file systems