-
-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: support drupal 11 #146
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThe updates primarily focus on enhancing compatibility with Drupal 11, including upgrading the development environment, adding new scripts, and updating existing configurations. Key changes include PHP version adjustments based on Drupal core versions, inclusion of tools like DDEV, and a general update of utilities to support the latest technologies. Changes
Assessment against linked issues
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Actionable comments outside the diff hunks (1)
.gitpod/drupal/drupalpod-setup/drupalpod-setup.sh (1)
Line range hint
186-186
: Correct the syntax issue with thethen
keyword.Ensure that there is a semicolon or a linefeed before the
then
keyword to avoid syntax errors in the script. This will ensure the script executes as expected without interruptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
# Add DDEV’s GPG key to your keyring | ||
RUN sudo sh -c 'echo ""' | ||
RUN sudo install -m 0755 -d /etc/apt/keyrings | ||
RUN curl -fsSL https://pkg.ddev.com/apt/gpg.key | gpg --dearmor | sudo tee /etc/apt/keyrings/ddev.gpg > /dev/null | ||
RUN sudo chmod a+r /etc/apt/keyrings/ddev.gpg | ||
|
||
# Add DDEV releases to your package repository | ||
RUN sudo sh -c 'echo ""' | ||
RUN echo "deb [signed-by=/etc/apt/keyrings/ddev.gpg] https://pkg.ddev.com/apt/ * *" | sudo tee /etc/apt/sources.list.d/ddev.list >/dev/null | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid using sudo
in Dockerfiles to enhance security.
Using sudo
in Dockerfiles can lead to potential security vulnerabilities and configuration errors. Consider running these commands as a non-root user or ensuring that the user context is appropriately managed without sudo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the code base is checked out to the core version prior to setting php_version so DrupalPod never gets the correct php_version for core.
|
||
# Before Drupal 10.2, we should use php 8.2, otherwise use php 8.3 | ||
if (( major_version < 10 )) || { (( major_version == 10 )) && (( minor_version < 2 )); }; then | ||
php_version="8.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The php_version seems to always get set to 8.2 when I launch with https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=10.2.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/tree/issue/NT-Drupal11-update
or https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=11.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/tree/issue/NT-Drupal11-update
maybe because drupal core has not been checked out to DP_CORE_VERSION yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=11.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/tree/issue/NT-Drupal11-update
should work.
I think that maybe then the condition on line 6 should check emptiness of DP_ISSUE_FORK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stand corrected, it still chose 8.2 rather than 8.3.
So I think we should not rely on the codebase checkout because it hasn't happened yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe if DP_CORE_VERSION does not match what is currently checked out, then ignore the current checkout for this script since it's an init script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see the Drupal 11 quickstart. If you ddev config --update
it sets to php8.3. But it can obviously be done with --php-version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rfay thank you! I am trying to support multiple drupal versions in drupalpod, and I thought adding .ddev/config.gitpod.yaml
based on what's needed is the easiest way, but I am open to change that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested Drupal core 11.x: https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=11.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/pull/146
- success
Tested freelinking 4.0.x using Drupal core 11.x: https://gitpod.io/#DP_PROJECT_NAME=freelinking,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_module,DP_MODULE_VERSION=4.0.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE=standard/https://github.com/shaal/drupalpod/pull/146
- success
I haven't done any manual regression testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drupal 10.2.x issue with issue fork branch: https://gitpod.io/new/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=drupal-3440560,DP_ISSUE_BRANCH=3440560-menu-is-created,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=10.2.x,DP_CORE_VERSION=10.2.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE=standard/https://github.com/shaal/drupalpod/pull/146
failed because composer tried to install 11.x, but couldn't, and it also chose php_version 8.3.
|
||
# Before Drupal 10.2, we should use php 8.2, otherwise use php 8.3 | ||
if (( major_version < 10 )) || { (( major_version == 10 )) && (( minor_version < 2 )); }; then | ||
php_version="8.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested Drupal core 11.x: https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=11.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/pull/146
- success
Tested freelinking 4.0.x using Drupal core 11.x: https://gitpod.io/#DP_PROJECT_NAME=freelinking,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_module,DP_MODULE_VERSION=4.0.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE=standard/https://github.com/shaal/drupalpod/pull/146
- success
I haven't done any manual regression testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke too soon. Regression testing an issue fork branch on 10.2.x failed.
|
||
# Before Drupal 10.2, we should use php 8.2, otherwise use php 8.3 | ||
if (( major_version < 10 )) || { (( major_version == 10 )) && (( minor_version < 2 )); }; then | ||
php_version="8.2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Drupal 10.2.x issue with issue fork branch: https://gitpod.io/new/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=drupal-3440560,DP_ISSUE_BRANCH=3440560-menu-is-created,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=10.2.x,DP_CORE_VERSION=10.2.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE=standard/https://github.com/shaal/drupalpod/pull/146
failed because composer tried to install 11.x, but couldn't, and it also chose php_version 8.3.
This reverts commit ff6f14e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the following scenarios successfully.
- 10.2.x issue fork regression test:
https://gitpod.io/new/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=drupal-3440560,DP_ISSUE_BRANCH=3440560-menu-is-created,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=10.2.x,DP_CORE_VERSION=10.2.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE=standard/https://github.com/shaal/drupalpod/pull/146
- 10.1.x , php 8.2 regression test:
https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=10.1.x,DP_CORE_VERSION=10.1.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/pull/146
- 11.x core test:
https://gitpod.io/#DP_PROJECT_NAME=drupal,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_core,DP_MODULE_VERSION=11.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE='standard'/https://github.com/shaal/DrupalPod/pull/146
- 11.x contrib test:
https://gitpod.io/#DP_PROJECT_NAME=freelinking,DP_ISSUE_FORK=,DP_ISSUE_BRANCH=,DP_PROJECT_TYPE=project_module,DP_MODULE_VERSION=4.0.x,DP_CORE_VERSION=11.x,DP_PATCH_FILE=,DP_INSTALL_PROFILE=standard/https://github.com/shaal/drupalpod/pull/146
Thank you! |
Fix #144, #145
In this PR:
Summary by CodeRabbit
New Features
composer-drupal-lenient
package to improve module compatibility with Drupal 11.Bug Fixes
Chores