Use a better container cache key

I think we have some options to improve this. The simplest is to hash the composer installed versions raw data. This means that the container will be rebuilt on any change to composer. We could achieve the same thing if we used the lock hash but that would mean we would need to read composer.lock whereas using InstalledVersions::getAllRawData() means that PHP can opcache everything. Another option would be have a composer plugin that could write the lock hash to a PHP file for us to read - in a similar way that composer writes the installed versions.

There are some side effects of this approach. One is any change to your composer installed versions will trigger a container rebuild even if a the module, theme or library is not Drupal installed yet. Another is because we take the root project reference into account if your root composer is checked in to git and this repo contains your custom code then any change to custom code will cause a container rebuild (if you do a composer install afterwards). For me this is a good thing.

Can we discover the class name of the autoloader, as that appears to contain the lockfile hash? This is presumably to work around similar problems with opcode caching.

    "content-hash": "7f3c203e5297306f9277da08961993f2",

then in vendor/composer/autoload_real.php:

class ComposerAutoloaderInit7f3c203e5297306f9277da08961993f2

The class is only used for initialization so there is nothing holding on to it after the autoloader is set up but you can find the name

> array_filter(get_declared_classes(), fn($class) => str_starts_with($class, 'ComposerAutoloaderInit'));
= [
    257 => "ComposerAutoloaderInit7f3c203e5297306f9277da08961993f2",
  ]

This is probably faster than serialize and hash?

@longwave this class does not always use the hash... locally on my core checkout where I work...

array_filter(get_declared_classes(), fn($class) => str_starts_with($class, 'ComposerAutoloaderInit'));
= [
    241 => "ComposerAutoloaderInitDrupal9",
  ]

I have nfi why InitDrupal9 :D

FWIW generating the hash takes 0.00072503089904785 Seconds

And that suffix is not reliable... it's generated like this:

            $suffix = $config->get('autoloader-suffix');

            // carry over existing autoload.php's suffix if possible and none is configured
            if (null === $suffix && Filesystem::isReadable($vendorPath.'/autoload.php')) {
                $content = (string) file_get_contents($vendorPath.'/autoload.php');
                if (Preg::isMatch('{ComposerAutoloaderInit([^:\s]+)::}', $content, $match)) {
                    $suffix = $match[1];
                }
            }

            if (null === $suffix) {
                $suffix = $locker !== null && $locker->isLocked() ? $locker->getLockData()['content-hash'] : bin2hex(random_bytes(16));
            }

See AutoloaderGenerator.php - I guess mine is a carry-over?!!?! No idea tbh.

The reason I decided to have a look at this is because of 🐛 Updating to 8.x-2.0-beta20 from 8.x-2.0-beta19 causes WSD Active and a feeling that it is unfair that core doesn't have to have empty updates for container rebuilds while contrib does. And that difference makes things harder to explain and remember.

Re #9 so removing vendor and regenerating gets me a class using the composer hash. But I think we've proved that that method is unreliable. I think if we want the most performant option we're going to need to have a composer plugin write this info for us into a php file.

Drupal9 is a remnant from #3254149: Remove config.autoloader-suffix from composer.json → - but yeah if it can be overridden and isn't guaranteed to change then it's not reliable here I guess.

Turns out writing the hash out is not tricky at all.

I just realised we don't want the hash from the composer.lock file - that's the content hash ie. composer.json hash. What we need need is a hash of the lock file contents - ie. something that changes when a version changes. So I've updated the code to do that.

@joachim reminded me that this solution could also be used to solve 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs work

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

It would be great to have some reviews on this...

> public static function getCode(PackageInterface $root_package, InstalledRepositoryInterface $repository): string {

We'll need to add a parameter to that when 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs work joins in to add its own data to the DrupalInstalled class, probably the ManageOptions object. But that's fine to happen in that other issue, as the class with this method is marked as @internal.

> scaffold does not run if you install Drupal from core git repo

How do we resolve this?

> scaffold does not run if you install Drupal from core git repo

Well this issue resolves this for this case because we're calling the preAutoload dump from core code now so it's way better. I don't think scaffolding has to run but there being a common preAutoload dump is necessary and a good thing. This issue introduces that and will make 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs work easier as you'll be able to rely on the presence of the constant.

Left a small comment on the MR.

The autoloader not being updated is, from my understanding causing quite some confusion when geocoder plugins, which are not modules, and are installed only with composer, are sometimes not detected. 🐛 How to add Gecoder 3.x providers Needs review

This would be great to land in 11.3.0

A couple comments on the MR for minor docblock issues, and some nits that are fine to go without. I also tested this locally and the apcu prefix and cache container keys change as expected with changes to composer packages or running composer install after a git commit.

This is great for core because it means that any updates that introduce new services just work. But it is painful for contrib because it has to always add empty updates to indicate that a container rebuild is necessary.

Another is because we take the root project reference into account if your root composer is checked in to git and this repo contains your custom code then any change to custom code will cause a container rebuild (if you do a composer install afterwards).

Do we need a CR for the above?

Also, is it possible there are projects out there that aren't built with core-recommended or scaffold? If so, would there be an error because the DrupalInstalled class doesn't exist?

@godotislate

Also, is it possible there are projects out there that aren't built with core-recommended or scaffold? If so, would there be an error because the DrupalInstalled class doesn't exist?

Is a great question. Core is just such a use-case. However I think we should make drupal/core dependent on drupal/core-composer-scaffold as that makes everything simpler and is fundamentally correct. This allows the instructions to be quite simple - see the CR https://www.drupal.org/node/3531162 →

> The DrupalInstalled.php file is located in the drupal in directory in the vendor directory.

This doesn't read right, not sure what it's supposed to say.

Made a few tweaks -- the README needs to mention this.

Also I'd like to bring more docs over from the Drupal root issue, which IIRC had more docs on the code that writes the file.

> file_put_contents($vendor_dir . '/drupal/DrupalInstalled.php'

What's the reason for putting it here rather than where core's files are put? Or in the webroot?

Looked at the CR and made edits, including the line mentioned in #26, but it'd be good to get other eyes on it now, too.
MR looks good to me as well.

A couple things outstanding from #27:

Also I'd like to bring more docs over from the Drupal root issue, which IIRC had more docs on the code that writes the file.

Is there anything left to be done here?

What's the reason for putting it here rather than where core's files are put? Or in the webroot?

Only guessing at the reasoning, but for one, it'd involve a new .gitignore entry then, right?

> Is there anything left to be done here?

Yes, I think so. I need to compare the two MRs.

> Only guessing at the reasoning, but for one, it'd involve a new .gitignore entry then, right?

It would be good to document the reasoning so future people don't have to guess :)

> Only guessing at the reasoning, but for one, it'd involve a new .gitignore entry then, right?

That is a good reason if that is the reason, but I'm not keen about putting something in vendor/$VENDOR. Things don't belong in that directly, they go in package subfolders.

What can't we put this in the web/core folder, which is gitignored in the recommended install template too?

vendor is where code is supposed to go and it is where our composer plugins are. This file is generated by a composer plugin. Also it's where composer put's its generated files.

We could add documentation along the lines of

    // Create the Drupal\DrupalInstalled class in the vendor/drupal directory.
    // vendor/drupal is used because it where Drupal's composer plugins are
    // installed and this file is analogous to composer's InstalledVersions file
    // which is written to vendor/composer. Also this file should not be 
    // committed to a project's version control repository.

But for me this documentation is not really necessary. Saying the composer places autoloadable files in vendor does not feel unexpected at all. Also any location outside of vendor will means we need to think git ignoring and deployment. You have to deploy your vendor directory somehow.

Maybe we only need to document on this issue? Ie. this comment.

Re the relationship between this and the drupal locations MR @joachim one question I have is do we ever expect a user to manual change the location written to the file or can we always rely on the composer plugin working it out correctly?

> vendor is where code is supposed to go and it is where our composer plugins are. This file is generated by a composer plugin. Also it's where composer put's its generated files.

Composer manages /vendor, and it puts packages in /vendor/VENDOR/PACKAGE. /vendor/composer is a special case, because it's composer's own folder.

Putting our own thing in /vendor/drupal, at the VENDOR level is completely unexpected, and it's creating another Drupalism.

> Re the relationship between this and the drupal locations MR @joachim one question I have is do we ever expect a user to manual change the location written to the file or can we always rely on the composer plugin working it out correctly?

The idea with 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs work was that a developer might change properties in composer.json such as the webroot. The DrupalLocations / DrupalInstalled file would only ever be generated automatically, based on those properties in composer.json, and you wouldn't ever change it manually.

I disagree the composer creating files in vendor is unexpected. But let's look for precedence...

\Http\Discovery\Composer\Plugin::preAutoloadDump actually writes a file in vendor/composer in certain situations which is even more surprising to me... but whatevs.
\Nevay\SPI\Composer\Plugin::dumpGeneratedServiceProviderData also writes a file to vendor/composer....

Hmmm... there is a pattern here - I guess we should be writing to vendor/composer ... also both of those plugins have the classname start with Generated which seems like a pattern...

@joachim what do you think about moving the file to vendor/composer and calling it GeneratedDrupalInstalled? One thing that's interesting to be me is that these plugins still use their own namespace - so their classes are \Nevay\SPI\GeneratedServiceProviderData and \Http\Discovery\Strategy\GeneratedDiscoveryStrategy. even though they are in vendor/composer.

I think given we can put it in whatever namespace we like maybe \Drupal\Composer\GeneratedDrupalData or something similar would work best.

@joachim that comment is a great addition about the file being generated.

@alexpott FWIW, I think the proposed changes in #34 are fine. I also think that it'd be good to get this in as early as possible for 11.3, and I don't think that the file location needs to be a blocker. We can create a follow-up as needed if there are problems with the file location, as long as it's resolved by 11.3.0-alpha1?

I would put it in the folder our own package is put in, which is WEBROOT/core. That's already going to be .gitignored.

> \Http\Discovery\Composer\Plugin::preAutoloadDump actually writes a file in vendor/composer in certain situations which is even more surprising to me... but whatevs.

I would talk to the maintainers of Composer about whether that is a good idea before doing it.

I've reached out to @naderman and @Jordi Boggiano (maintainers of Composer) in Symfony slack to ask. No reply yet.

Here's a transcript of my conversation with @Jordi Boggiano.

alexpott
@naderman @Jordi Boggiano I’ve got a composer plugin that needs to write a class during pre autoload dump. The contents of the class are based on information from composer. I’ve noticed that other composer plugins that do something similar write files to vendor/composer/ which feels really interesting to me and I don’t know why they do that. Is this the recommended place for plugins to write utility classes like this that depend on information that composer has?
Thanks for any pointers :slightly_smiling_face:

Jordi Boggiano
@alexpott hmm.. it is kinda the only reserved/safe place for composer to write things because we know for sure vendor/composer/autoload_real.php for example will never exist due to a package named composer/autoload_real.php as we control that vendor. Why people dump their stuff in there I do not know, can't say that I condone this but I assume they mostly do it because it feels like the place to dump random stuff because composer does it

Jordi Boggiano
it's fine as long as people use unique enough file names..

Jordi Boggiano
like GeneratedServiceProviderData and GeneratedDiscoveryStrategy are not strictly names I would consider namespaced/unique enough but hey, people take the chances they want to take

Jordi Boggiano
that's why plugins are great.. I don't need to maintain them if shit goes wrong:smile:

alexpott
@Jordi Boggiano thanks for reply. My own implementation dumped the file in my own namespace directory in vendor which feels safer. I was wondering why these other plugins did not take that approach and if there was a good reason because yeah the potential for file name clashes feels an odd risk to take.

Jordi Boggiano
yeah using your own vendor sounds safer to me

Given the above discussion with @Jordi Boggiano, I still feel that using vendor/drupal is the best place. I disagree that placing files in vendor is a Drupalism. I would argue that placing files outside of vendor is by far the bigger Drupalism. Code that is in vendor is much easier to place outside of the webroot and is therefore my secure. In an ideal world core would be in vendor and we would not be having this discussion.

Ok so that's /vendor/composer out :)

My concern with /vendor/drupal instead of /web/core is DX.

Could someone with this branch checked out confirm that an IDE can find the file containing the DrupalInstalled class from a place in the code where it's referenced? (PHP language features in my IDE hammer my laptop, sorry.)

If that's not the case, then it's not an obvious place for a developer to look for that file.

> In an ideal world core would be in vendor and we would not be having this discussion.

In which case, we'd put it in vendor/drupal/core. Which is the package folder, which is where I'm proposing we put it -- just that our package folder goes in a funny place.

Once this issue is in, and 🐛 Wrong DRUPAL_ROOT with non-standard code structure Needs work is in, then we get a lot closer to having core in /vendor.

Could someone with this branch checked out confirm that an IDE can find the file containing the DrupalInstalled class from a place in the code where it's referenced?

PHPStorm finds the class, no problem.

The ValidatableConfig job in the build is failing, but it seems like that's because the job doesn't run composer install again after the switch back to the latest MR commit, and not an issue specific to this MR.

RTBC +1.

> scaffold does not run if you install Drupal from core git repo

Ok so I just thought of a way in which we fix this.

If we write this file to /web/core, then an initial version of DrupalInstalled.php can be in the git repo for core, in that location, containing default values for the constants.

Then the scaffold plugin overwrites it.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

If we write this file to /web/core, then an initial version of DrupalInstalled.php can be in the git repo for core, in that location, containing default values for the constants.

For my core development workflow, this would mean there'd be a modified DrupalInstalled.php file in my repo a lot of the time, which is an annoyance (probably minor) when rebasing etc.

We could alternatively protect against DrupalInstalled somehow being missing with a class_exists check:

class_exists(DrupalInstalled::class) ? DrupalInstalled::VERSIONS_HASH : \Drupal::VERSION

> For my core development workflow, this would mean there'd be a modified DrupalInstalled.php file in my repo a lot of the time

If you're installing Drupal core direct from a git clone, then scaffolding doesn't run, does it?

And anyway, I was suggesting that scaffolding would leave the file alone if it's the git version.

(Also, you already have composer.json modified whenever you install additional tools -- that's why I made https://github.com/joachim-n/drupal-core-development-project)

Please let's not overwrite a file from core that would be the worst of all world. I really do not think there is an issue with vendor/drupal containing Drupal classes, not from an IDE or a conceptual basis.

Also with this change the root composer.json and a Drupal scaffold enabled site are running the same preAutoLoad dump script - which is a good thing because it'll improve the performance of sites because a site of classes that are required to make respond to a cache hit will now not be autoloaded they will just be included.

I rebased the MR to fix conflicts with 11.x

Nightwatch test failed, needs rerunning most likely.

Nightwatch passed, everything lgtm now.