Add setting to move comment form after comments

Will need some form of upgrade path for the new settings.

Also tagging for sub-maintainer for their thoughts.

The upgrade hook should set the new config to false.

If I go to the olivero settings, change nothing, click save, and do a config export I'll get a new change now because the schema is different.

Looks good for me!

Thanks folks we still need a subsystem maintainer opinion here - would suggest pinging one of the olivero maintainers in #frontend in slack

Left some comments on the MR - thanks folks

Not olivero maintainer, but it's ok with me.

Committed 26e593a and pushed to 11.x. Thanks!

feedback incomming

Added feedback to the MR.

The config changes look v nice... a couple of other thoughts...

We can add an update test to \Drupal\Tests\olivero\Functional\Update\OliveroPostUpdateTest
I think we can test the functionality in core/tests/Drupal/Nightwatch/Tests/Olivero/oliveroCommentTest.js

I bet it needs change record, moreover it reminds me that we have comment type entity to store this kind of data

Personally I find comment field formatter or comment type is a better place to store position of the form.

Also it needs to revisit D6/7 migrations as there was related setting

@andypost pointed out that the comment field already includes a setting for 'form location' but that's only if its on the same page or a separate reply page. I think its ok for a theme to make a setting for how to position the form when its on the same page.

    $element['form_location'] = [
      '#type' => 'checkbox',
      '#title' => $this->t('Show reply form on the same page as comments'),
      '#default_value' => $settings['form_location'],
    ];

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.

+1 to get in, just can't test 11.x with Bartok properly for BC but it's contributed theme now

Hi
I have reproduced the issue on Drupal 11 with the Olivero theme.
But the MR is falling for Drupal 11

Testing steps:
Just create a new node with comments

Attaching screenshot for reference

Hi
I have reproduced the issue on Drupal 11 with the Olivero theme.
The MR is applied cleanly...

Testing steps:
Just create a new node with comments

Test Result:
I have added 2 comments, but after applying the MR, the added comments are not visible only the count of comments is visible.

Attaching Screenshots

I cannot reproduce issue from #34

All works as expected

Attaching Screenshots too

Don't think this has been reviewed by an Olivero maintainer yet so tagging for that.

I think this is really something that should be configurable centrally in Drupal core, probably as a formatter setting. However we'd still need to remove the hard-coding of the form position in Olivero if we do that, but it would mean the position moves to the formatter settings instead of Olivero. Moving back to needs review for discussion.

should be configurable centrally in Drupal core, probably as a formatter setting

++ to get more opinions as there setting and default value when new theme enabled is tricky and same time option for comment form gonna be added in #2546116: Allow a sitebuilder to change the Comment form heading

Re #27

theme to make a setting for how to position the form when its on the same page

consider a case when you have 2 comment fields for the entity and and wanna configure it at comment field/type level...

Looking back I find @catch choice is preferable as initial intent was #1920044: Move comment field settings that relate to rendering to formatter options

99% themes render comment form after comments list, only olivero before.

Some sites will want to render the comment form before the comments and will be overriding themes, so the formatter would help in that direction too.

Hello
I have reproduced the issue for Olivero theme on Drupal 11.
The MR is applied successfully.

Testing steps-
Created new node and added comments.

Test Result:
Results are same before and after applied MR.
Check attachments.

I think it all means that Olivero doing too many things that is not ready in comment module, like hardcoding comment form placement and comment count #2031903: Support per comment field comment count tokens via https://git.drupalcode.org/project/drupal/-/blob/11.x/core/themes/oliver...

@sagarmohite0031 why did you remove all the tags?

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

False positive

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Fixed PHPStan issues reported by the bot in #51 but still some tests are failing that need to be fixed.

Thanks!

tested locally. works as expected
+1 RTBC

The MR for this issue was identified as having a new Kernel/Functional test class that did not have the #[RunTestsInSeparateProcesses] attribute. A deprecation warning is now issued in the case of these omissions. I've rebased the MR added the attribute to prevent this from being committed as-is and accidentally breaking tests on HEAD. Because this is a minor change to test metadata and the tests are passing I am leaving the issue's status as RTBC.

@catch asked about this in Slack. I understand why it would make more sense to put this outside of the theme if this was a feature request for “allow users to choose whether the comment form is before or after,” but no one seems to need that. This issue only exists because Olivero made an odd design choice, so to me a global setting, which increases the complexity of this, seems unnecessary.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

There were two issues with the MR:

1. theme_get_setting() is deprecated as of Drupal 11.3.0.
2. The new Choice constraint lacked a choices key which is deprecated in Symfony 7.4.

Fixing these were minor changes to correct easily verified problems and I considered self-RTBCing the issue again. But then I figured that three unreviewed commits to the MR in a row was probably a bridge too far. So I'm setting this back to Needs Review.

Not product manager but it’s been a few months hoping restoring the RTBC will get their attention

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.

Rebased. This required making minor updates to remove the $has_trusted_config parameter and update the imported fixture to the new one from 11.3.0.

Pointing out that this still needs signoffs and I have added that to the remaining task.

Reading the comments I see that there is not agreement on the implemented solution as shown by these comments.

• #26 the preferences if for changing the comment field formatter or comment type.
• #27, the current implementation is preferred.
• #36, catch prefers that this is "configurable centrally in Drupal core, probably as a formatter setting".
• #59, no one needs a global setting, it is just Oliver that made an "odd design choice"

If the current implementation is agreed to the change record needs updates. It should start with the change. And the branch/version need to be updated, and the reference to Bartik should be removed.

@smustgrave Re:

Not product manager but it’s been a few months hoping restoring the RTBC will get their attention

The issue is also still tagged for subsystem maintainer review for Olivero, which product management likely needs to make an informed decision, so the correct status is NR.

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.