Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix: do not explicitly set the backdrop_drupal_compatibility and update_free_access settings, fixes #6090 #6091

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: do not explicitly set the backdrop_drupal_compatibility and update_free_access settings, fixes #6090 #6091

wants to merge 2 commits into from

Conversation

klonos
Copy link
Contributor

@klonos klonos commented Apr 13, 2024 •
edited

The Issue

As described in #6090, if these settings are explicitly set in settings.ddev.php, trying to set them in settings.php or settings.local.php never takes.

How This PR Solves The Issue

This PR removes the lines in settings.ddev.php that explicitly set these two settings, which allows the values to be set in settings.php or settings.local.php (which unlike Drupal is provided by default OOTB in Backdrop).

Manual Testing Instructions

  1. Install a vanilla Backdrop CMS site using the DDEV recipe.
  2. Edit your settings.php file or the settings.local.php file and set $settings['backdrop_drupal_compatibility'] = FALSE;.
  3. Try reading that setting via settings_get('backdrop_drupal_compatibility') (for instance using the devel module and dpm() or something) -> regardless of how that setting is configured, it is always returned as TRUE 👎🏼
  4. Apply this PR/patch -> try reading the setting again -> this time it should return the value as set in the settings.php file or the settings.local.php file 👍🏼

Automated Testing Overview

N/A

Related Issue Link(s)

Fixes #6090

Release/Deployment Notes

N/A

@klonos klonos requested a review from a team as a code owner April 13, 2024 09:22
@klonos klonos mentioned this pull request Apr 13, 2024
Open
1 task
@github-actions GitHub Actions
Copy link

github-actions bot commented Apr 13, 2024 •
edited

Download the artifacts for this pull request:

See Testing a PR.

rfay
rfay reviewed Apr 13, 2024
View reviewed changes
Copy link
Member

@rfay rfay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine with me. But it seems like it would be better to

  1. Include settings.local.php after settings.ddev.php (fixing that)
  2. In settings.ddev.php say "if not set then set them" and keep the settings.

But we really just want to do what works best for Backdrop folks. Glad to have you here.

@rfay
Copy link
Member

rfay commented May 1, 2024

Hoping to hear a response from you about this @klonos - I do think there's an easier way.

If I don't hear back by May 15 or so I'll close this.

@klonos
Copy link
Contributor Author

klonos commented May 1, 2024

Hey @rfay 👋🏼 ...apologies for the late reply here; we have been working on things in Backdrop core since today is the feature freeze for the upcoming 1.28.0 minor release, which is slated for May 15. There has been too much activity to keep up with, and there will continue to be a lot of effort/focus in the next couple of weeks on fixing bugs and improving all the features that got in before the feature freeze.

I promise to loop back to this after all that "release frenzy" has subsided (if not sooner than that).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rfay rfay rfay left review comments

Assignees
No one assigned
Labels
bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Do not explicitly set the backdrop_drupal_compatibility and update_free_access settings in settings.ddev.php
2 participants
@klonos @rfay