Problem/Motivation

#3587118: Remove content types from the standard install profile and recipe left the comment module dependency in the standard profile / recipe. We kept it there to manage scope. We should remove it.

Steps to reproduce

Proposed resolution

Remove the comment module dependency.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

The standard install profile and recipe does not depend on comment module anymore.

LLM disclosure

LLM was used to accelerate the development of this MR, everything was deeply reviewed though.

CommentFileSizeAuthor
#23 3588276-nr-bot_hlu_1rg9.txt91 bytesneeds-review-queue-bot
#19 3588276-nr-bot_husy9k6a.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3588276

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

gábor hojtsy created an issue. See original summary.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Parent issue: #3588274: Remove Tags vocabulary from Standard profile and recipe » #3159848: [Policy] Always install Drupal with Standard on the UI, pared down of use case specific elements (content types, node listing, commenting, theme)
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Postponed

Postponed on #3159848: [Policy] Merge the minimal and standard install profiles into a starting point recipe landing first.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Postponed » Active
Issue tags: +Lean Starter Experience

The content types are removed on main in #3587118: Remove content types from the standard install profile and recipe, we can move on with this actially.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes

gábor hojtsy opened merge request !15807

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review

Green and ready :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue summary: View changes

Adding LLM disclosure.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Pretty simple. Did a standard profile install and comments wasn’t there :)

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Parent issue: #3159848: [Policy] Always install Drupal with Standard on the UI, pared down of use case specific elements (content types, node listing, commenting, theme) » #3591027: [META] Remove use case specific elements from Standard and Node module. Always install with Standard. Keep Minimal for CLI use only.

Adjust parent to implementation issue from policy issue.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Minor but we might as well fix here:

I applied the MR diff, and then grepped the standard profile, and we have this comment in the performance test:

  // Node page second request: Zero placeholders (no comment form on test content type).
    $this->drupalGet($node->toUrl());
    $this->assertBigPipePlaceholderReplacementCount(0);

I think we can just remove the bit in parenthesis, it's still true that there are zero placeholders, and there's no comment form, we just don't need to mention that because there wouldn't be a comment form anyway. I would have done this in an MR suggestion myself but then I realised I'm not sure how to do that on code that's not already in the MR diff at all.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs work » Reviewed & tested by the community

Needed a trivial rebase following #3588274: Remove tags vocabulary from standard profile and recipe and I removed that one code comment bit @catch asked in #11. :) I think this remains RTBC.

  • catch committed 004e5335 on main 
    task: #3588276 Remove comment module dependency from standard profile...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to main, thanks! Will need a backport MR for 11.x

catch closed merge request !15807

smustgrave opened merge request !15874

smustgrave’s picture

smustgrave commented
Status: Patch (to be ported) » Needs review

This one will need review.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Needs review » Reviewed & tested by the community

Thanks for working on the backport. Migrate test updates look good to me. Rest of the MR looks like the one landed in main.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3588276-nr-bot_husy9k6a.txt91 bytes

The 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.

smustgrave closed merge request !15874

smustgrave opened merge request !15964

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Had to start a new branch as there were 100s of conflicts (not sure where though)

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3588276-nr-bot_hlu_1rg9.txt91 bytes

The 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.

smustgrave’s picture

smustgrave commented
Status: Needs work » Reviewed & tested by the community

Rebased

  • catch committed 996c3830 on 11.4.x 
    task: #3588276 Remove comment module dependency from standard profile...

  • catch committed 39e95282 on 11.x 
    task: #3588276 Remove comment module dependency from standard profile...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed the backport to 11.x and 11.4.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !15964

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Issue tags: +11.4.0 release highlights
gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

Added change record at https://www.drupal.org/node/3606437