Wrong DRUPAL_ROOT with non-standard code structure

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

New branch & MR for 11.x

Have not reviewed.

But appears to have composer error and can't trigger retest.

Fixed the errors.

joachim → closed merge request !1922

I only found one real question in regards of a missed dirname(__DIR__, 2); in one of the files. There is other places we might want to use our new found power instead of some of the references through __DIR__. Most seem in tests though which i'd not touch probably.

I've run through this whole issue and went through the MR intensely. This seems like an elegant solution that will enable us to take a step forward in how core is structured.

Resolved all points.

The latest version of MR looks great. Added suggestions for improvement.
The only thing that I want to clarify is the upgrade path for the existing projects. I reviewed CR, and there is only mention of the new API, but no upgrade steps. As I understand, existing projects need to update the autoload section of the composer.json file, right?

> The only thing that I want to clarify is the upgrade path for the existing projects. I reviewed CR, and there is only mention of the new API, but no upgrade steps. As I understand, existing projects need to update the autoload section of the composer.json file, right?

Oh that's a good point!

In the MR, any code that uses the \Drupal\Composer\Plugin\Locations\DrupalLocation class also has a fallback for if that class is not found.

That fallback is for the benefit of existing installations, but also for plain git clones, where there is no drupal scaffolding done and so the DrupalLocation won't get written.

So actually, the CR needs to be changed, and possibly the docs as well, to say this: you have access to DrupalLocation IF you know you are in a project that uses drupal/core-drupal-locations -- in other words, in code that you own in your project, or in a Composer project template that requires drupal/core-drupal-locations and sets up scaffolding. Generally, in thid-party code, you should use DrupalKernel::getAppRoot().

Ok, so PR looks good. I set NW for CR updates.

MR needs to rebase

Updated the README and the CR.

GitlabCI (wow, 4 times faster than DrupalCI) was not happy with CSpell check. I added an entry to a dictionary. The changes look good to me for RTBC

'definining' is a typo, not a word :)

I'll fix.

Fixed.

I think the BC layer in bootstrap.inc is not correct - it should use Drupal\Locations\DrupalLocation; and not use Drupal\Composer\Plugin\Locations\DrupalLocation; - also the comment in bootstrap.inc is out-of-date.

Plus I think people need to guidance about whether to deploy DrupalLocation.php or should it be it .gitgnore like the other scaffolded files.

> I think the BC layer in bootstrap.inc is not correct

Fixed, and same problem in DrupalKernel too.

> Plus I think people need to guidance about whether to deploy DrupalLocation.php or should it be it .gitgnore like the other scaffolded files.

It can be either -- it's documented in the code, but I'll add it to the README:

 * To ensure the project's codebase is portable, the generated class must not
 * use absolute paths. It should instead use the __DIR__ constant, which in the
 * generated class will give the project root.

> should it be it .gitgnore like the other scaffolded files

We could do that, but code to add files to .gitignore is in the drupal/core-composer-scaffold and it's marked @internal. Is it ok for the locations plugin to call the scaffold plugin given they're both Drupal core?

We could do that, but code to add files to .gitignore is in the drupal/core-composer-scaffold and it's marked @internal. Is it ok for the locations plugin to call the scaffold plugin given they're both Drupal core?

Maybe this new composer plugin should be part of scaffold - as it is very similar to the autoload.php we're scaffolding.

I did look at doing that at one point, but I felt that its function was sufficiently different from Scaffold to count as feature creep - Scaffold just copies files from one place to another according to a recipe. This generates a class.

alexpott → closed merge request !2459

alexpott → closed merge request !483

alexpott → closed merge request !473

alexpott → closed merge request !591

alexpott → closed merge request !182

Hiding all the patches.

DrupalCon Lille conversation -- things to change:

1. Merge new Composer plugin into Scaffold - we've agreed that Scaffold's purpose is generally 'Make the Composer project happy with Drupal's weirdnesses'. It already generates the autoload.php file.
2. Look at https://getcomposer.org/doc/04-schema.md#classmap instead of PSR autoload.
3. Be clearer in the CR about what existing sites need to or can do -- cover various different setups.

I rebased and fixed conflicts in composer.lock and core/lib/Drupal/Core/Test/FunctionalTestSetupTrait.php. In FunctionalTestSetupTrait.php, this line had been removed altogether: chdir(DRUPAL_ROOT), see 5ef2211afc. In this MR, the line was changed to chdir($app_root). I don't have much context for either issue so I kept the change from this issue.

I updated the error messaging for coding standards, but I didn't address any of the comments in the MR. I was just trying to get this back to green.

(Also, sorry Joachim, I didn't notice Assigned until after I started committing things.)

Thanks!
I probably should have unassigned myself a while ago -- haven't got the mental space to dive back into one at the moment. Thanks for keeping it ticking over!

It might be worth seeing if we can do without the chdirs() which are removed in 5ef2211afc.

I'm trying to rebase this, and I'm not managing to update Composer from lock, and I don't remember how I did it previously!

> COMPOSER_ROOT_VERSION=11.0-dev composer update --lock

says:

Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Root composer.json requires drupal/core == 11.9999999.9999999.9999999-dev (exact version match), it is satisfiable by drupal/core[11.x-dev] from composer repo (https://repo.packagist.org) but drupal/core[11.0-dev] from path repo (core) has higher repository priority. The packages from the higher priority repository do not match your constraint and are therefore not installable. That repository is canonical so the lower priority repo's packages are not installable. See https://getcomposer.org/repoprio for details and assistance.

and

> COMPOSER_ROOT_VERSION=11.9999999.9999999.9999999-dev composer update --lock

works, but then the version set in composer.lock is all wrong.

You have be in the 11.x branch to update the lock file.

So is the right way to do it:

1. merge 11.x into feature branch
2. Update lock file
3. switch to feature branch, commit changes
4. switch to 11.x and do a git reset --hard to put it back

?

Aha, that's worked. Thanks!!

> 2. Look at https://getcomposer.org/doc/04-schema.md#classmap instead of PSR autoload.

That's not going to work. I've tried it, and any Composer operation which happens before the DrupalLocation.php is written produces this error, multiple times:

> Could not scan for classes inside "DrupalLocation.php" which does not appear to be a file nor a folder

That happens when doing `composer drupal:scaffold` and probably will happen when first installing Drupal with Composer.

The problem is that 'classmap' autoloading looks in the given list of files to find classes to register. That's unlike 'psr-4' autoloading, which merely declares directories to Composer, and it will only look for files when a class is about to be loaded.

So with psr-4 it's fine to declare a file that doesn't exist yet, but with classmap it's not.

@alexpott - your concern was that the namespace could clash with other things. What if make it more specific?

Instead of:

> namespace Drupal\Locations;

we have:

> namespace Drupal\Composer\Locations;

The Drupal\Composer namespace is used by things in our /composer folder, nobody else should be using that namespace.

Moved all the code to the scaffold plugin.

I've started trying to get the gitignore to work and can't quite get it working :/

joachim → changed the visibility of the branch 1792310-locations-class-writer-plugin-11 to hidden.

Figured out gitignore.

Made a new branch & MR as the old branch name wasn't accurate any more now that the new functionality is being added to the existing drupal/core-composer-scaffold Composer plugin.

I've hidden the old branch.

New MR is: https://git.drupalcode.org/project/drupal/-/merge_requests/8216

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.

Rebased.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.