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.

Finally got all tests passing!!!!

Deprecation messages are outdated.

Fixed. Back to NR.

Looks good for me. I can't wait to see it in core for implementation of additional project template in the scope of #1672986: Option to have all php files outside of web root. →

Updated proposed resolution in IS.

HTTP request to core/modules/statistics/statistics.php (yay! special case!)

IS should not mention removed in 11.x statistics module

Updated IS for #247.

Thanks for working to complete this old issue!

I read the issue summary and skimmed through the comments. There is work to do for the last comment, so setting to NW.

I read through the comments in the MR and left some suggestions. Would it not be simpler to use the same language and style that the ProxyBuilder uses for building files? I think the class LocationsClassTemplate could be in the manager file. I am not sure that 'manager' is an appropriate name. I'll have to look again at another time.

I don't see history here for why there is a change record 'Obsolete ...'. What ever the reason, in the future just update the draft CR on an issue. Having unwanted extras adds to the difficulty in managing the change records.

> Would it not be simpler to use the same language and style that the ProxyBuilder uses for building files?

Could you be more specific?

I've previously looked a bit at ProxyBuilder, and the whole area of proxy services is one I find to be pretty badly documented. I don't really understand why ProxyBuilder exists or how it does what it does. Also, ProxyBuilder has to write much more complex classes than in this case -- here we just need to replace tokens with strings. ProxyBuilder has to write methods and all sorts.

> I think the class LocationsClassTemplate could be in the manager file.

I wanted to keep it separate for clarity. That way the class that holds the code for the DrupalLocations file is just about that code.

> I am not sure that 'manager' is an appropriate name. I'll have to look again at another time.

I followed the same pattern as ManageGitIgnore, also in the scaffold plugin. ManageGitIgnore is responsible for creating a .gitignore file. So ManageDrupalLocations is responsible for creating a DrupalLocations file.

> I don't see history here for why there is a change record 'Obsolete ...'. What ever the reason, in the future just update the draft CR on an issue. Having unwanted extras adds to the difficulty in managing the change records.

The approach was completely changed. I don't think it would make sense for the node history of the CR node to go from saying one thing to saying something utterly different. The obsolete CR can be deleted.

I'm not sure about your suggested change from 'app root' to 'application root'. I made the changes and then thought to check core -- in 11.x there are LOTS of uses of the term 'app root'. So I'm wondering about whether to revert those commits I just made, and keep consistent with what's already in core.

Also, your suggestion for this doesn't work:

   *   The web root of the project. This is either defined in the project root
   *   composer.json, or taken to be the project root by default.

It needs to be a single sentence, I think.

I did a light review of my previous comments and the comments in the MR. (It is noisy here and it is hard to think). There is just the one sentence we are working to agree on. I have made a new suggestion.

This time I noticed that that new classes are not declaring stricts types, Is there a reason? And there are new methods which are not defining return types. Should they? Oh, that noise is helping me to make mistakes. I didn't see your comment and the use of 'app root' in comments and I applied my suggestion. Sorry, about that.

Regarding ProxyBuilder, \Drupal\Component\ProxyBuilder\ProxyBuilder includes embedded strings that are placed in the generated files. I think it is much simpler, avoids another class and method, and with the bonus it easier to read (at least for me). As for the other I hadn't considered ManageGitIgnore. That all can wait for an opinion from another committer.

@joachim thoughts on #251?

Thanks for the bump @smustgrave! I'd forgotten about this needing work from me!

> There is just the one sentence we are working to agree on. I have made a new suggestion.

I've taken that, and added more detail, because I think it's important to say what happens here if the root composer.json doesn't specify the web-root.

> This time I noticed that that new classes are not declaring stricts types, Is there a reason?

That's not yet a standard AFAICT -- 🌱 [policy, no patch] Use declare(strict_types=1) in new code Active

> And there are new methods which are not defining return types. Should they?

The only one I can see failing that is getApplicationRoot(). Fixed.

> I didn't see your comment and the use of 'app root' in comments and I applied my suggestion. Sorry, about that.

I'm not sure what your opinion is here, as I can't see your changes in the MR any more -- did you conclude it's ok to use 'app root' for now?

> Regarding ProxyBuilder, \Drupal\Component\ProxyBuilder\ProxyBuilder includes embedded strings that are placed in the generated files.

Do you mean core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php?

I actually find that much harder to read and work with, certainly while developing how the written file should look.

I work iteratively, so I get the DrupalLocation written, look at what I want it to have, or what I need to change, and then go and make changes to LocationsClassTemplate. It helps me if LocationsClassTemplate looks mostly like the code that I want to output!

I tried to test this today but the diff does not apply to 11.x and yet GitLab says it does. There are also 2 failing tests.

The approach was completely changed. I don't think it would make sense for the node history of the CR node to go from saying one thing to saying something utterly different. The obsolete CR can be deleted.

It is perfectly fine for the change record history to reflect the changes to the solution. In fact, it should be preserved to do exactly that, to show the history. And, in doing that, it also prevent making work for someone to cleanup the change records.

Thank for the changes in the MR.

Rebased on 11.x.

Sorry about the new CR -- not sure how best to fix that.

I talked to @longwave about this at DrupalCon Barcelona last week.

1. He asked about whether the DrupalLocations value is an absolute or relative path, in case the DrupalLocations class is committed and deployed. It's relative, precisely for that reason. I've added a note about that to the issue summary.

2. On the matter of terminology 'app root' vs 'application root', he suggested we could use 'application root' in documentation, and $app_root as a variable name. I looked into doing that, but I found that 'app root' is used in LOTS of places in existing code comments. So I think this MR should stay consistent with that, and a follow-up issue can look at that.

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.

The MR might need some more work in the context of recipes (probably recipecommand.php in particular). I am using https://github.com/joachim-n/drupal-core-development-project and i've tried to apply a recipe recently. due to the symlinking of the core folder between the repos/drupal and the docroot web folder i ran into issues when i've executed php core/scripts/drupal recipe ../recipes/tour_core -vvv in the docroot web folder. There i ran into the error The specified database connection is not defined.
I've ssh'ed into the container and went to /var/www/html/web/core/scripts. there i tried php -a and ran print_r(__DIR__); which returned /var/www/html/repos/drupal/core/scripts instead. so executing the recipe command was not using the sites folder in web but in repos , which doesnt have any database credentials provided and therefore the error.
If you try instead of running that php command in the web folder ,drush recipe instead, drush manages that the database is found and the site is correctly bootstrapped based on the web folder instead of falling back to the repos folder. BUT i then run into another error: The following extensions are missing and are required for this recipe: tour. the tour module is installed in the web folder context but not available in the repos context. so it looks like the recipe command is probably falling back and relying onto repos/drupal instead of web in recipecommand.php in line 66 . I'll set the issue back to needs work.

*I've debugged and explored the problem at Drupal Dojo Austin with @rocketeerbkw and @leeotzu last night.