I have this drupal project, drupal also installed with composer and the Drupal root folder is nested in Composer root folder. In simple_fb_connect.install it is checked if the dependency (facebook/graph-sdk) exists. When looking for the installed.json file, after finding the one in drupal root folder, it will not look further up in Composer root folder, where all the packages are listed. My suggestion would be removing the check in simple_fb_connect.install because it can be a source of errors. As long as the module is installed with composer, we are sure that the dependency is also installed. Thus this error-generating unnecessary check I believe should be removed.
It is clearlly stated that the Drupal 8 version should be installed with Composer.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | complete_dependency_check-2873333-12.patch | 3.63 KB | ChrisHng |
Comments
Comment #2
ChrisHng commentedComment #3
ChrisHng commentedComment #4
ChrisHng commentedComment #6
ChrisHng commentedComment #7
ChrisHng commentedComment #9
ChrisHng commentedComment #10
ChrisHng commentedComment #11
masipila commentedI'm reluctant to remove the hook_requirements check. You are right that it is clearly stated that the module needs to be installed with Composer but without this hook_requirements check the issue queue of this module will be full of howto-questions.
As long as Drupal core does not have the functionality that contribs can use to check if the required composer dependencies are met, we need to have our own hook_requirements check. See #2647108: Modify installation time requirement check to use the mechanism provided by core, when it will become available.
So instead of removing the hook_requirements check, let's try to modify the check so that it works also in the use case that you have. Is it only a question that where do we read the installed.json from?
Cheers,
Markus
Comment #12
ChrisHng commentedI have completed the checks, so that it would check the first two levels.
Comment #13
ChrisHng commentedComment #14
masipila commentedHi,
I've been travelling quite much lately so I was able to review the patch only now. By reading the source of the patch I'm concerned that we are causing regression to drupal-scaffold installs where DRUPAL_ROOT is in /web subdirectory. Please see #2841887: Install fails if DRUPAL_ROOT is /web/. I have not tested this so I'm just saying that I'm concerned. I'm not saying that this is the case but we need to make sure that my concern is not valid. :)
We should be supporting the following three scenarios:
1. Manual installation
Current logic is that we check if DRUPAL_ROOT/vendor is a directory.
If it is, we read the installed.json from
2. Drupal-scaffold installation
Current logic is that we check if DRUPAL_ROOT/vendor is NOT a directory. If this is the case, we get the parent directory of DRUPAL_ROOT using dirname().
We then check the installed.json from
3. Now we have this third scenario that you reported. Starting from DRUPAL_ROOT, what is the relative path where the installed.json is located at?
I think we need to write these scenarios (with relative paths from DRUPAL_ROOT) where installed.json can be found in each of the scenarios so that simple_fb_connect_read_packages() remains maintainable.
Cheers,
Markus
Comment #15
masipila commented@ChrisHng, would you have time to comment the question in my previous post?
To be more specific, is the relative location of installed.json from DRUPAL_ROOT this or something else:
Cheers,
Markus
Comment #16
masipila commentedI finally had time to test the different combinations / installation mechanisms. I did not have any problems with the hook_requirements check with any of the these so I'm postponing this issue as I need further information on the problematic installation mechanism.
1. Drupal installed with Composer using drupal-composer/drupal-project as the Composer project template
The recommended way to install Drupal 8 with Composer is to use drupal-composer/drupal-project as the project template.
composer create-project drupal-composer/drupal-project:8.x-dev my_site_name_dir --stability dev --no-interaction2. Drupal can also be installed with Composer using drupal/drupal
composer create-project drupal/drupal my_site_name_dir3. Drupal was installed manually from the tarball
Conclusion: the hook_requirements check that we currently have covers all these installation scenarios. As mentioned in #11, as long as Drupal core does not provide a mechanism for contrib modules to check that the composer requirements are met, we need to have our own hook_requirements check. If this hook_requirement check is not working with some installation scenario, please re-open this issue and provide detailed steps on a) how you installed Drupal and b) what is DRUPAL_ROOT in this case and c) what is the path to installed.json starting from DRUPAL_ROOT.
Cheers,
Markus