Closed (fixed)
Project:
Drupal core
Version:
10.3.x-dev
Component:
book.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
11 Jan 2024 at 05:42 UTC
Updated:
20 Dec 2024 at 07:44 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
quietone commentedComment #3
quietone commentedComment #5
quietone commentedComment #6
spokjeSo our decision to not remove the deprecated modules related CSS in 10.3.x, but rather in 11.x bites us here.
core/themes/olivero/config/optional/block.block.olivero_book_navigation.ymlhas a dependency on book, which triggers a deprecation in\Drupal\KernelTests\Config\DefaultConfigTest::testThemeConfig.Not quite sure what to do here, maybe special case this situation in
DefaultConfigTest, either specific for olivero and books, or better in general?Comment #7
catchIf modules can provide default config for themes, we could move the block over and not the CSS, but... can they even do that??
Comment #8
larowlanYeah dependencies supports thèmes sonth so that should be doable - nice idea!
Comment #9
spokjeOk, so I moved the dependency and block settings over to a optional config yaml in the book module, that didn't break too much, but I like to be sure this is the correct approach before trying to fix the new test-failures.
Comment #10
catchFrom looking at a couple of the test failures they look like ones we'd probably have to deal with when we move book out of core - like comparing default config etc. if that's the case then I think it makes sense to do it this way.
Comment #11
spokjeTestfailures where due to me not moving the whole block but trying to be clever.
(Trust me, that never works out...)
Straight up move of the
block.block.olivero_book_navigation.ymlto the book module appeases all tests.Comment #12
spokjeComment #13
smustgrave commentedCould a CR be added?
Comment #14
spokjeAh sorry, thought one was already there.
It wasn't, added one.
Comment #15
smustgrave commentedThanks! Reviewing this like I did Forum and believe everything has been addressed.
Comment #16
larowlanWe need a stable release of the contrib module before we can proceed here
Thanks
Comment #17
quietone commentedComment #18
andypostContrib module created but one blocker still here
Comment #19
catchJust committed the blocker.
Comment #20
andypost@pwolanin Now it needs to move code to contrib module according to https://www.drupal.org/about/core/policies/core-change-policies/module-o...
After that RTBC +1
Comment #21
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #22
catchThis still needs a stable release of the contrib module per #16 at https://www.drupal.org/project/book
Comment #23
smustgrave commentedWill do one by 10.3 for sure. Think we were waiting for #3422862: Add validation constraints to book.settings and #3424509: Update MigratePluginManager to include both attribute and annotation class to land to avoid multiple subtree splits.
But if we can do the sub-tree split without those and just backport manually without a full sub-tree split we can do a release today.
Comment #24
catch#3422862: Add validation constraints to book.settings may have its own dependencies, so I don't think the book subtree split should wait on it. If it had already gone in, it would have been worth waiting for, but it's still got open questions so might not even land in 10.3.x
#3424509: Update MigratePluginManager to include both attribute and annotation class looks a bit closer so might still be worth trying to push that through first.
However we're coming up close to the 10.3 beta deadline, and still need to do module removals and update test fixture changes in the next three to four weeks. I'm not sure if/how many fixtures include book module, but if they do, then it might be better to just go ahead here.
Comment #25
smustgrave commentedMigrate one just landed so we can do the sub tree split.
Comment #26
smustgrave commentedA 1.0.0 release has been made for book
Rebased this MR.
Comment #27
smustgrave commentedI really just rebased this. Work was already done so think I'm good to RTBC.
Comment #30
catchReviewed this again, all looks straightforwards. Some core modules I am pleased about deprecating and have been trying to get rid of for years, but I still like and use book module so this is a shame, but it's just not used as much as it was, and there are other options in contrib these days too. I'll need to install the contrib book module for at least one site.
Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!
Comment #31
quietone commentedPublish CR