Problem/Motivation

At the moment our container cache key is constructed from
$parts = ['service_container', $this->environment, \Drupal::VERSION, Settings::get('deployment_identifier'), PHP_OS, serialize(Settings::get('container_yamls'))];

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.

Steps to reproduce

Update any contrib module.

Proposed resolution

I think we can do better and use a hash of the composer lock file data in the container key. This will mean more container rebuilds but things will be more reliable.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3509069

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

alexpott created an issue. See original summary.

alexpott’s picture

Version: 11.1.x-dev » 11.x-dev
alexpott’s picture

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.

longwave’s picture

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

alexpott’s picture

Status: Active » Needs review
longwave’s picture

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? json_encode() is also faster than serialize() iirc (but json_decode is slower than unserialize)

alexpott’s picture

@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

alexpott’s picture

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.

alexpott’s picture

The reason I decided to have a look at this is because of #3509039: Updating to 8.x-2.0-beta20 from 8.x-2.0-beta19 causes WSD 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.

alexpott’s picture

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.

longwave’s picture

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.

alexpott’s picture

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

alexpott’s picture

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.

alexpott’s picture

Issue summary: View changes
longwave’s picture

@joachim reminded me that this solution could also be used to solve #1792310: Wrong DRUPAL_ROOT with non-standard code structure

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

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.

alexpott’s picture

Status: Needs work » Needs review

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

joachim’s picture

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

We'll need to add a parameter to that when #1792310: Wrong DRUPAL_ROOT with non-standard code structure 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?

alexpott’s picture

> 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 #1792310: Wrong DRUPAL_ROOT with non-standard code structure easier as you'll be able to rely on the presence of the constant.

smustgrave’s picture

Status: Needs review » Needs work

Left a small comment on the MR.

ekes’s picture

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. #3153678: How to add Gecoder 3.x | 4.x providers

alexpott’s picture

Status: Needs work » Needs review

This would be great to land in 11.3.0

godotislate’s picture

Status: Needs review » Needs work
Issue tags: +Needs change record

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?

alexpott’s picture

Status: Needs work » Needs review

@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

joachim’s picture

> 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.

joachim’s picture

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?

godotislate’s picture

Issue tags: -Needs change record +Needs change record review

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?

joachim’s picture

> 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 :)

godotislate’s picture

Status: Needs review » Needs work
joachim’s picture

> 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?

alexpott’s picture

Status: Needs work » Needs review

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?

joachim’s picture

Status: Needs review » Needs work

> 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 #1792310: Wrong DRUPAL_ROOT with non-standard code structure 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.

alexpott’s picture

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.

alexpott’s picture

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

godotislate’s picture

@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?

joachim’s picture

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.

alexpott’s picture

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

alexpott’s picture

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

alexpott’s picture

Status: Needs work » Needs review

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.

joachim’s picture

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 DrupalInstalled 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 #1792310: Wrong DRUPAL_ROOT with non-standard code structure is in, then we get a lot closer to having core in /vendor.

godotislate’s picture

StatusFileSize
new100.96 KB

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.
Screenshot of PHPStorm showing information about DrupalInstalled class

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.

joachim’s picture

> 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.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

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.

godotislate’s picture

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

joachim’s picture

> 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)

alexpott’s picture

Status: Needs work » Needs review

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

godotislate’s picture

Nightwatch test failed, needs rerunning most likely.

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

Nightwatch passed, everything lgtm now.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

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.

alexpott’s picture

Status: Needs work » Reviewed & tested by the community

Fixed the merge conflict with 11.x

catch’s picture

Status: Reviewed & tested by the community » Needs review

One question on the MR, which was also discussed a bit back in February (but didn't look up the thread, just found it with ctrl-f for 'class_exists' on this page.

godotislate’s picture

I posted. suggestions on the MR to use class_exists per #52.

ressa’s picture

Thanks for working on this, it sounds like it may help clear APCu caches, after switching from using a deprecated core module, to its contrib replacement?

Adding a few related issues, with examples of the problems a stale APCu cache can result in.

mxr576 made their first commit to this issue’s fork.

mxr576’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record review

The class_exists() failsafe check makes sense to me as well - I've applied that suggestion.

I've also reviewed and tested the patch. The change record exists, so I'm removing the "Needs change record" tag.

The final implementation heavily relies on Composer, which raises an interesting question about potential new patterns for custom module development. Some developers (myself included) already register web/modules/custom as a local path repository and install custom modules via Composer. This approach:

  • Enables custom module dependencies to participate in dependency resolution
  • Makes modules more portable across projects
  • After this change, would also include custom modules in deployment identifier generation

For custom modules under version control, I assume $package->getSourceReference() returns the git commit hash on a project code hosted in VCS. If so, this would mean that changes to custom module service definitions would automatically trigger container identifier change, which seems like a beneficial side effect of this implementation.

This workflow enhancement could become a recommended best practice for custom module development in Composer-based Drupal projects. Anyway, a potential follow up on this change.

The current test failure is unrelated: #3539366: Default DB transaction isolation set to read-committed breaks InstallerIsolationLevelExistingSettingsTest test

nicxvan’s picture

Status: Reviewed & tested by the community » Needs work

Needs a rebase for the failure then i think it can be rtbc based on 56.

godotislate’s picture

Status: Needs work » Needs review

Rebased. There's one class_exists() suggestion that wasn't applied thought, and I left it as is in case it wasn't applied for a reason

mxr576’s picture

and I left it as is in case it wasn't applied for a reason

No, I have just missed it.

mxr576’s picture

Status: Needs review » Reviewed & tested by the community
needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

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.

godotislate’s picture

Status: Needs work » Reviewed & tested by the community

Rebased for merge conflict.

needs-review-queue-bot’s picture

Status: Reviewed & tested by the community » Needs work
StatusFileSize
new91 bytes

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.

mxr576’s picture

Status: Needs work » Needs review

Rebased, resolved conflicts on composer.json and composer.lock.

mxr576’s picture

Status: Needs review » Reviewed & tested by the community
catch’s picture

Status: Reviewed & tested by the community » Fixed

Still have some vague undefined reservations here, but hopefully those are unfounded, certainly nothing concrete any more - I think the class_exists() check resolves most of them anyway.

Committed/pushed to 11.x, thanks!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.

  • catch committed 93238804 on 11.x
    Issue #3509069 by alexpott, longwave, joachim, smustgrave, godotislate,...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.