Problem/Motivation

See #3566536: [meta] eliminate core .module files

Proposed resolution

  • Move the logic inside \Drupal\comment\CommentForm::preview()
  • Deprecate comment_preview()

Remaining tasks

None.

User interface changes

None.

Introduced terminology

None.

API changes

The comment_preview() function is deprecated.

Data model changes

None.

Issue fork drupal-3566879

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

claudiu.cristea created an issue. See original summary.

claudiu.cristea opened merge request !14295

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Assigned: claudiu.cristea » Unassigned
Status: Active » Needs review

Ready for review.

nicxvan’s picture

nicxvan commented

Oh this is an interesting approach.

I think it makes sense, do we know why this was split up the way it was?

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

I think it makes sense, do we know why this was split up the way it was?

I have tried to understand the same thing but didn't found an answer. It could be a relic from a previous refactoring

quietone’s picture

quietone commented
Title: Move comment_preview() logic to CommentForm (+deprecate) » Deprecate comment_preview() logic to CommentForm

I think we can just say deprecate.

dcam’s picture

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

I couldn't find any issues with the MR. The comment preview continued to work correctly after applying the MR. So this looks good to me.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

I couldn't find any issues with the MR. The comment preview continued to work correctly after applying the MR. So this looks good to me.

Has anyone tested that calling `comment_preview()` still works?

nicxvan’s picture

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

That's a good point this is complicated enough that is probably worth confirming the deprecation pathway is tested.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Instead of adding a useless test, I would rather restore the original code there.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review

As I've mentioned in #11, instead of testing that, better preserve the original (but deprecated) code. Reverted the code in comment_preview() so it doesn't needs test

nicxvan’s picture

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

I think that is a valid way to handle this.
I think this is ready again!

I also pulled this down and manually tested.

larowlan closed merge request !14295

  • larowlan committed 6f12b759 on 11.x 
    feat: #3566879 Deprecate comment_preview() logic to CommentForm

By:...

  • larowlan committed 2c5006a3 on main 
    feat: #3566879 Deprecate comment_preview() logic to CommentForm

By:...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed to main and backported to 11.x

Thanks folks

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.

Status: Fixed » Closed (fixed)

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