The line
\Drupal::service('module_installer')->install(['social_api']);
In the requirements hook, just shouldn't be there. The dependency in the info.yml file is enough.

Right now, if you want to use social_auth in an install profile, this creates issues, since it then tries to install social_api way too early.

Patch will follow soon.

CommentFileSizeAuthor
#15 2943924-15.patch866 bytesgvso
#10 dependency-declaration-10.patch743 bytesMaskyS
#8 dependency-declaration-8.patch1.09 KBMaskyS
#4 2943924-4-dependency_declaration.patch641 bytesdaften
#3 2943924-3-dependency_declaration.patch575 bytesdaften
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daften created an issue. See original summary.

daften’s picture

daften CreditAttribution: daften at District09, Picabit commented
Title: Dependency declaration should be done in info file, not in the install hook » Dependency declaration should be done in info file, not in the requirements hook
Issue summary: View changes
daften’s picture

daften CreditAttribution: daften at District09, Picabit commented
FileSize
2943924-3-dependency_declaration.patch575 bytes

Added a patch for this

daften’s picture

daften CreditAttribution: daften at District09, Picabit commented
FileSize
2943924-4-dependency_declaration.patch641 bytes

Former patch didn't work, better patch in attachment. Installing requirements in hook_requirements is simply not done.

gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented

The intention of that part of the code was not to install Social API because it is a dependency, but rather to check if the correct composer library was installed. But in order to call the checkLibrary method, Social API must be installed.

For reference, check #2847739: Provide method to check if correct composer library is installed

daften’s picture

daften CreditAttribution: daften at District09, Picabit commented

It's still the wrong method to do it. The patch checks if social_api is installed and doesn't mess with the normal flow of things. Since the requirements hook is triggered in several places, the check will happen at the right time too.

gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented

I see..
My concern with the patch's solution is that the library won't be checked if Social API is not installed. Yes, this is actually not a big issue since when installing the module, you do it with composer which automatically downloads the dependencies.
I believe this method was implemented in Social API when composer wasn't completely integrated and there were some reports that were related to a missing library, so this approach might be considered 'outdated' at this point.

Any thoughts on this?

MaskyS’s picture

MaskyS CreditAttribution: MaskyS at Google Code-In commented
Status: Active » Needs review
FileSize
dependency-declaration-8.patch1.09 KB

I agree with the point daften is making, but gvso is right too. As the patch is right now it won't check the installed library if Social API itself isn't installed. I wouldn't consider this outdated because I think we should let users install Social API manually without going through composer.

I went through implementations of hook_requirements by core modules. Here's something similar in the attached patch. Would be great if it could be tested.

gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented
Status: Needs review » Needs work

Just found out that there's no need to install nor to check if Social API is installed. However, I noticed that Social Auth can be installed even if PHP League OAuth2 is not downloaded.

MaskyS’s picture

MaskyS CreditAttribution: MaskyS at Google Code-In commented
Status: Needs work » Needs review
FileSize
dependency-declaration-10.patch743 bytes

Now that I think about it, you're right! Drupal itself would verify if Social API is installed and abort if not since the dependency is listed in .info file. So let's just remove the check, and see why Social Auth is installing without PHP OAuth2 in a separate follow-up issue?

gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented

Yes, this is the right approach, and I agree we should work on the problem I mentioned in a separate issue. However, we might need to solve the problem before applying this patch.

gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented
Status: Needs review » Needs work
gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented
Related issues: +#2494073: Prevent modules which have unmet Composer dependencies from being installed
gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented
Title: Dependency declaration should be done in info file, not in the requirements hook » Remove library check in the requirements hook
Version: 8.x-2.0-beta3 » 8.x-2.x-dev
Category: Bug report » Task
Parent issue: » #3038664: [META] Remove support for library check
gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented
Status: Needs work » Needs review
FileSize
2943924-15.patch866 bytes

  • gvso committed c53c600 on 8.x-2.x 
    Issue #2943924 by daften, MaskyS, gvso: Remove library check in the...
gvso’s picture

gvso
Primary language Spanish
CreditAttribution: gvso at Google Code-In, Google Summer of Code commented
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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