Problem/Motivation

In 9.x we deprecated some code in book.module, it's time to remove it.

Steps to reproduce

Proposed resolution

Remove the code, ensure no usages remain.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#2 3263391-2.patch15.18 KBlongwave

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Active » Needs review
StatusFileSize
new 3263391-2.patch15.18 KB
tmaiochi’s picture

tmaiochi commented
Assigned: Unassigned » tmaiochi

I'll review this

tmaiochi’s picture

tmaiochi commented
Assigned: tmaiochi » Unassigned
Status: Needs review » Reviewed & tested by the community

Steps performed:
(1) Installed module
(2) Reproduced the issue.
(3) Applied patch.
(4) Code review on changes.
(5) Tested again with patch, issue resolved.
The patch removed all deprecated code from Book module, and everything still working as far as I can test it

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3263391-2.patch, failed testing. View results

  • catch committed f431117 on 10.0.x 
    Issue #3263391 by longwave: Remove deprecated code from book.module
catch’s picture

catch
he/him
Primary language English
commented
Status: Needs work » Fixed

@tmaiochi this issue is removing deprecated code that doesn't run when book module is installed, it would only run if another module depended on it. So, there's no need for manual testing, and no bug to reproduce - posting those as steps actually makes it look like less review happened rather than more.

To get contribution credit for reviewing an issue like this, there are really only a couple of things that need doing:

- A visual review of the patch just to check it's only removing code that's deprecated, and that the code is deprecated for removal in Drupal 10 (it's not impossible to have code deprecated for removal in Drupal 11).

- A quick grep/look over of book module to make sure there's no deprecated code left in it once the patch is applied. For example if a test is removed that uses a fixture, it's possible the fixture could be removed if it's not used by any other tests.

So, I haven't assigned contribution credit for this issue, but hopefully this will help with reviewing similar issues in the future.

Patch looks good and the test fail is unrelated.

Committed f431117 and pushed to 10.0.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.