Problem/Motivation
Some Install profiles could need more resources - PHP requirements in terms to run.
Because of
- packages/libraries used,
- amount of packages/libraries
- coding
- processes resources
So maybe we need a specific minimum PHP version and/or a specific minimum PHP memory limit.
Steps to reproduce
It's just not possible this overriding - not implemented.
Proposed resolution
Update system_requirements
to get install profile "PHP requirements".
And if they exists and use them, overriding Drupal default "PHP requirements".
Remaining tasks
- Do we need a change record for this?
User interface changes
API changes
Data model changes
Release notes snippet
Original Summary report
imho there should be a way to change the constants defined for the system requirements
as 'DRUPAL_MINIMUM_PHP' or 'DRUPAL_MINIMUM_PHP_MEMORY_LIMIT'.
there could be some complex install profiles that requires "more power" from the system.
for example there's a Commerce Kickstart issue about this : #1763632: The install needs to check Kickstart requirements before starting.
Comment | File | Size | Author |
---|---|---|---|
#57 | 1772316-nr-bot.txt | 150 bytes | needs-review-queue-bot |
Issue fork drupal-1772316
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #1
vasikehere is a patch that allows thatbad approach
are there other system requirements constants that could be changed?
Comment #2
vasikehere is a new approach for changing the php system requirements.
it uses the install_profile_info definition.
so what it's needed it's to specify new values in the info file of the profile
Comment #4
vasikerebuilt the patch, i hope it's the right one
Comment #5
vasikeand here is the patch for Drupal 8
Comment #6
sunComment #7
saltednutLooks like some D7 install profiles have been using #4 and it no longer applies to HEAD as of 7.22. :(
Added tags Needs reroll but there's no way to specify thats a D7 reroll since this is also being tracked as an 8.x issue - sorry.
Comment #8
cweagans#5: allow_change_system-requirements-1772316-5.patch queued for re-testing.
Comment #10
cweagansWe need to get this into D8 before we worry about D7. I requeued the patch in #5 (I doubt it'll pass - last test run was in September). If that fails, lets reroll that, get it committed, then deal with D7.
Comment #11
cweagansAh, cross posted with the bot. So yeah, need to reroll that patch.
Comment #12
cweagansLet's give this a try. I moved around your new block of code in system.install so that we're not calling drupal_get_profile() multiple times - now it's just called once at the beginning of the function and used throughout.
Comment #13
cweagansComment #15
tobyontour CreditAttribution: tobyontour commentedMade system.install default to DRUPAL_MINIMUM_PHP_MEMORY_LIMIT if it's not set in the profile (this is what caused the previous patch to fail).
Comment #16
tobyontour CreditAttribution: tobyontour commentedPatch failed to attach to last post.
Comment #17
Tor Arne Thune CreditAttribution: Tor Arne Thune commentedI'm not sure about "Drupal requires at least PHP %version.", as this is not true if the profile uses a different max version than Drupal. Drupal with [name of profile] requires this version, yes. Also the same issue in the message about the memory limit.
Comment #18
jsacksick CreditAttribution: jsacksick commentedHere's the rerolled patch for d7, against the latest dev
Comment #20
hswong3i CreditAttribution: hswong3i commented#18: drupal7-allow_change_system-requirements-1772316-18.patch queued for re-testing.
Comment #22
jsacksick CreditAttribution: jsacksick commentedThe issue is tagged with the 8.x version which means that the patch will obviously fail..., that's why I didn't change the status earlier... I rerolled the patch for Commerce Kickstart
Comment #23
hswong3i CreditAttribution: hswong3i commentedPatch(es) reroll via all latest dev.
Comment #24
hswong3i CreditAttribution: hswong3i commentedComment #25
David_Rothstein CreditAttribution: David_Rothstein commentedNote: #309040: Add hook_requirements_alter() is another way to do this that should be a lot more flexible.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedAlso seems closely related to #309457: Allow profiles to specify required memory in .info file.
Comment #27
manuelBS CreditAttribution: manuelBS commentedIn some installations we have problems with mysql max_allowed_packet size set to low. Wouldn't ist make sense to set this settings also as requirement if needed?
Comment #28
xtfer CreditAttribution: xtfer commentedUpdated for D7, as the patch was not applying.
Comment #30
xtfer CreditAttribution: xtfer commentedSwitching to 7 to run the test.
Comment #31
xtfer CreditAttribution: xtfer commented28: drupal-7.x-allow_profile_change_sys_req-1772316-28.patch queued for re-testing.
Comment #32
xtfer CreditAttribution: xtfer commentedBack to 8.x
Comment #33
jhedstromComment #34
rpayanmComment #45
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPatch does not apply
Comment #46
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThe patch no longer applies, so adding tag for re-roll.
Comment #48
RassoniReroll against 9.4.x dev branch.
Comment #49
RassoniComment #50
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedRe-rolled #34 for 9.5
Comment #51
ranjith_kumar_k_u CreditAttribution: ranjith_kumar_k_u at Zyxware Technologies commentedComment #52
immaculatexavier CreditAttribution: immaculatexavier as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedCustom Commands have been fixed, and an interdiff against #51 has been attached for 9.5.x
Comment #53
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #55
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #57
needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commentedThe 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.
Comment #58
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #59
Bhanu951 CreditAttribution: Bhanu951 as a volunteer commentedComment #60
smustgrave CreditAttribution: smustgrave at Mobomo commentedThis issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.
FYI didn't read all the comments but #56 appeared to be a rebase.
This could use an issue summary update as I'm not fully understanding the use case or the "why" maybe the test case will have show that. But the issue summary should mention the proposed solution and other fields per default template.
Comment #62
vasikeUpdated the MR
- Fix for
php_memory_limit
- Test for overriding PHP version and memory limit
Update also the issue summary
Comment #63
smustgrave CreditAttribution: smustgrave at Mobomo commentedCleaning up tags.
Think the last thing is to get a change record written with an example how this would be set.
Comment #64
vasikeUnfortunately, i think the MR is not ready ... yet
2 things:
1. It would be nice to have also RECOMMENDED_PHP ... overridden.
2. What if the values are less than "default" - Drupal provided? Should we allow that.
Another thought:
Wondering if
Drupal\Core\Utility\PhpRequirements
shouldn't be the place or getting the "right" PHP Requirementsinstead of
system.install
...but maybe i'm just "overthinking" ...