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.

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

Drupal 10 support #161

Merged
merged 18 commits into from
Jun 22, 2023
Merged

Drupal 10 support #161

merged 18 commits into from
Jun 22, 2023

Conversation

stephen-cox
Copy link
Member

@stephen-cox stephen-cox commented Jan 25, 2023

This adds Drupal 10 support. The two major changes that come with this are:

Now installs CKEditor 5

CKEditor 4 is currently deprecated and removed in D10. We probably want to check the config for this. Do we perhaps want to move to CKEditor 5 now?

Media config is now in install rather than optional

This hasn't caused any further issues with installing other modules, but should be tested with current install and Microsites as well.

@finnlewis
Copy link
Member

Very cool @stephen-cox !

@andybroomfield
Copy link
Contributor

CKEditor 4 is currently deprecated and removed in D10. We probably want to check the config for this. Do we perhaps want to move to CKEditor 5 now?

What are the potentional issues here? Assume this would install for anyone on new sites, others would still need to upgrade? Is this something that would justify a 3.x branch of core?

@andybroomfield
Copy link
Contributor

andybroomfield commented Jan 26, 2023

Also did #158 and the followup get resolved, ie. moving the media into install. Assume it did based on

This hasn't caused any further issues with installing other modules, but should be tested with current install and Microsites as well.

but be good to confirm.

@finnlewis
Copy link
Member

Discussed in Merge Monday, continue to test and review this week, not ready for merge yet.

  • test in microsites profile

Putting into draft

@finnlewis finnlewis marked this pull request as draft January 30, 2023 14:10
@stephen-cox
Copy link
Member Author

stephen-cox commented Feb 16, 2023

I have found a potential issue with moving config in LocalGov Media to install.

Much of this config also exists in the Standard Drupal profile that comes with core. If you try to enable LocalGov Media when using the Standard profile it will fail with an error about config already existing.

Is there a use case for installing LocaGov modules in a site using the Standard profile? If so we're going to have to think of a way around this. For example, keep config in optional and add a hook that installs the config if using the localgov profile.

@andybroomfield
Copy link
Contributor

@stephen-cox yes, I don't think we should assume the profile is how everyone installs modules, the goal was that modules can be installed independantly which really could mean installing them in a otherwise standard profile site (I've also been testing modules this way).

Also I thought the arguments around requiring or installing modules in the profile was that profile would set up how the site is intended to use, but those who didn't want could install modules independantly.

As a workaround, could the already in standard media config be moved to optional? or some module use a config/override directory and place config in there?

@ekes
Copy link
Member

ekes commented Feb 16, 2023

I for one have chatted to people about installing: Directories, Step-By-Step, and Alert Banner outside of the profile. I'd expect Subsites might be another.

@ekes
Copy link
Member

ekes commented Mar 6, 2023

Is the sticking point on this one the media config location?

@stephen-cox stephen-cox marked this pull request as ready for review June 5, 2023 13:57
@ekes ekes self-requested a review June 12, 2023 10:48
Copy link
Member

@ekes ekes left a comment

Choose a reason for hiding this comment

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

New installs will get ckeditor 5 works fine. It is available for switching for existing installs.
Tested against an existing d9 localgov test db and nothing adverse. Permissions and ckeditor5 on a new install look good. Tests pass.

Is there some way of tracking where the permissions are getting installed and doing that step-wise? Even maybe not removing them all from here yet till the others are committed?

@finnlewis
Copy link
Member

finnlewis commented Jun 12, 2023

Discussing in Merge Monday with Ekes.

Currently we have lots of merge requests for Drupal 10, some approved, but planning to not merge these yet and discuss to review how to get the D10 branches working without breaking the D9 installs.

Suggesting that we either need to:

  • focus on the D9 branches, keeping them working while making releases that start to support D10
  • co-ordinate merge and release across all projects.

Note: we have a Technical Governance meeting on Wednesday where we will focus on confirming the strategy for this.

Ekes suggested listing all modules:

  • projects that will support D9 and D10
  • projects that will need different branches.

See localgovdrupal/localgov#480

@stephen-cox stephen-cox merged commit 59805fc into 2.x Jun 22, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants