Problem/Motivation
As it currently stands, install profiles cannot insert their own requirements during the install phase.
drupal_check_profile grabs the info file from the install profile, and loops through all the dependencies to check for requirements, however it forgets to check its own requirements.
Supplied patch adds the profile to the end of the dependency array (temporarily) to check itself for requirements.
Proposed resolution
https://api.drupal.org/api/drupal/core%21includes%21install.inc/function...
Remaining tasks
Write a test for this to prevent regressions in the future.
Contributor doc on writing tests: https://drupal.org/contributor-tasks/write-tests
User interface changes
API changes
Data model changes
As it currently stands, install profiles cannot insert their own requirements during the install phase.
drupal_check_profile grabs the info file from the install profile, and loops through all the dependencies to check for requirements, however it forgets to check its own requirements.
Supplied patch adds the profile to the end of the dependency array (temporarily) to check itself for requirements.
Comment | File | Size | Author |
---|---|---|---|
#23 | install_profile_requirements_check_object_exists_on_install.patch | 754 bytes | scott_euser |
#9 | I1971072.patch | 546 bytes | AjitS |
#8 | I2108339.patch | 546 bytes | AjitS |
#5 | install_profile_requirements_on_install.patch | 546 bytes | Riaan Burger |
install_profile_requirements_on_install.patch | 572 bytes | japerry | |
Comments
Comment #1
xjmThanks @japerry! :)
Moving to 8.x first and tagging for backport, as per our backport policy.
Comment #2
star-szrTagging to create a new patch for this against 8.x. If possible it would be nice to test if this is still an issue on 8.x as well, and the test coverage can be worked on as well.
Docs for writing automated tests: https://drupal.org/contributor-tasks/write-tests
@japerry if you want to work on this just reassign.
Comment #3
Riaan Burger CreditAttribution: Riaan Burger commentedTested the issue using Drupal 8; It is still an issue; Tested as follows:
git clone of 8, added this code:
To file_requirements() in core/modules/file/file.install which resulted, as expected, in an error during install.
Added the same code to a new function in core/profiles/standard/standard.install as follows:
Expected a new error during install but did not get one.
Manually applied the patch to core/includes/install.inc which resulted in expected behaviour.
The issue is still valid and the patch still solves the issue.
Unless you are on it japerry, I'd like to try to tackle this one...
Comment #4
Riaan Burger CreditAttribution: Riaan Burger commentedComment #5
Riaan Burger CreditAttribution: Riaan Burger commentedComment #6
Riaan Burger CreditAttribution: Riaan Burger commentedComment #8
AjitSApplied the tests as mentioned in #3, above. The patch seems to solve the problem.
Re-rolling it again.
Comment #9
AjitSWrong issue name in the patch.
Comment #11
Riaan Burger CreditAttribution: Riaan Burger commentedAjitS, this was my first try at working on core stuff. Does this mean (I'm guessing from that you did) that the patch should be named after the issue node ID? Was that what caused all the patches above to fail.
Comment #12
AjitSRiaan Burger,
As far as I know, patch name doesn't have any impact on its "passing" or "failing". However, generally some standards are recommended to be followed while naming a patch. It is generally in the format
[issue_name]-[short-description]-[issue-number]-[comment-number].patch
More reading about Submitting patches.
Comment #13
Riaan Burger CreditAttribution: Riaan Burger commentedAjitS, do you have any idea why the first patches failed then? It looked to me like it was because of the file name initially (containing install.patch in the name). The patches do look the same to me, the failed and successful ones. Thanks for helping.
Comment #14
Daniel Bornman CreditAttribution: Daniel Bornman commentedAlso tested with the code from #3 above and confirming the patch solved the issue.
Comment #15
star-szrGreat! It should be possible to write a test for this so we can prevent regressions in the future.
Contributor doc on writing tests: https://drupal.org/contributor-tasks/write-tests
Comment #16
mgiffordThis still a concern in D8? Unassigned issue too.
Comment #17
no_angel CreditAttribution: no_angel as a volunteer commentedMaybe similar issue https://www.drupal.org/node/2608408?
Comment #18
no_angel CreditAttribution: no_angel as a volunteer commentedComment #19
nils.destoop CreditAttribution: nils.destoop as a volunteer and at Wunder commentedNot completely similar, but very close to it :)
Comment #21
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedIt's possible this patch needs additional work - see #2486083-21: The following module is missing from the file system: standard and subsequent comments for a bug that this patch causes (at least the Drupal 7 version of it... not sure about Drupal 8).
Comment #23
scott_euser CreditAttribution: scott_euser commentedThanks for the work on this! Helped me solve an issue. The installation profile I used was missing declaring it's dependencies so the patch still failed for me.
Perhaps rather than just loading the dependencies from the profile we can double check that the module object is loaded and if not, throw an error that helps the user know what dependency is missing so they can update their installation profile accordingly?
Attached patch as an example.
Comment #36
quietone CreditAttribution: quietone at PreviousNext commentedThis was a bugsmash triage issue a few days ago. At that time lendude commented that it was probably fixed by a later issue, #2309731: drupal_check_profile() does not invoke the profile's hook_requirements(). Today, I read that issue and the associated change record. I now also think this was fixed in that issue.
Therefor I am closing this as outdated.