Beta phase evaluation

-

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because simplifies code introduced in 8.x...
Issue priority Normal because planned clean-up
Prioritized changes The main goal of this issue is removing previously introduced code that, if used, means that configuration is being created programatically rather than using CMI .
Disruption Disruptive for contributed and custom modules if they use the method - however this was only introduced in 8.x.

Problem/Motivation

Helper method CommentManagerInterface::addDefaultField() is actively used by tests.
Usage in forum.install and standard.install removed in favour of shipped config files.

Proposed resolution

Let's mark the method as deprecated at least by pointing that comment field should be added via config file.
Move code from CommentManagerInterface::addDefaultField() to trait.
Usage in tests implemented via CommentTestTrait::addDefaultCommentField()

Remaining tasks

1. remove usage form *.install files and add default fields
2. agree on removal or deprecate
3. patch&commit

User interface changes

none

API changes

Removal of CommentManagerInterface::addDefaultField() in favour CommentTestTrait::addDefaultCommentField()

CommentFileSizeAuthor
#28 2357199-comment-trait-26_.patch52.26 KBandypost
#26 2357199-comment-trait-26.patch52.26 KBandypost
#14 interdiff.txt25.13 KBandypost
#14 2357199-comment-trait-15.patch52.25 KBandypost
#13 2357199-comment-trait-13.patch27.12 KBandypost
#11 removeaddDefaultField-2357199-1.patch34.26 KBnaiduharish
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Title: Consider CommentManagrInterace::addDefaultField() as deprecated and remove » Consider CommentManagerInterface::addDefaultField() as deprecated and remove
Issue summary: View changes
Issue tags: +Novice

Minimal patch to remove the usage from standard and forum install files

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
Parent issue: » #2188287: Split CommentManager in two

At runtime core needs less functions that we have now

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue tags: +@deprecated
naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented
Assigned: Unassigned » naiduharish
Issue tags: -@deprecated +@deprecated @SprintWeekend2015Queue

I am working on it.

naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented

Hi @andypost,

I have tried looking into the issue, but I haven't found "addDefaultField()" in forum.install and standard.install files.

Also "CommentManagerInterface::addDefaultField()" is been used in 6 files. So do we need to deprecate the the method and remove from all the tests it has been used.

Thank you.

naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented
Assigned: naiduharish » Unassigned
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue tags: -@deprecated @SprintWeekend2015Queue +@deprecated, +SprintWeekend2015

tags can be confusing.

Thank you for working on this issue.

Note, tags should be separated with a comma, not a space.

The Queue tag was used to line up issues before the sprint. When you work on an issue please add: SprintWeekend2015
(#2407325: preparing for a sprint, would be good to have a list of candidate issues)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

@naiduharish yep, the method used in tests only so I suppose better to move it to trait (probably CommentAddDefaultFieldTrait) that used in tests

naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented

@YesCT Thanks for the feedback, would take care of it here on.

@andypost Thanks for explanation, would work on it.

naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented
Assigned: Unassigned » naiduharish
naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented
FileSize
removeaddDefaultField-2357199-1.patch34.26 KB

Hi @andy,

I have renamed "addDefaultField()" function as "commentDefaultFieldTrait()" in the test files and the CommentManagerInterface.

naiduharish’s picture

naiduharish CreditAttribution: naiduharish commented
Assigned: naiduharish » Unassigned
Status: Active » Needs review
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
2357199-comment-trait-13.patch27.12 KB

I mean this.
Suppose we need to deprecated the functions first.
PS: patch removes functions for testing purposes

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
2357199-comment-trait-15.patch52.25 KB
interdiff.txt25.13 KB

Fix the rest

The last submitted patch, 13: 2357199-comment-trait-13.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

oh yeah

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

We need a beta-eval here

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

added, CRs to update
https://www.drupal.org/node/2100015
https://www.drupal.org/node/2112417
https://www.drupal.org/node/2285633

Probably needs new CR about trait

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Yep we need that CR

This is a good change because it encourages people to use CMI properly.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Title: Consider CommentManagerInterface::addDefaultField() as deprecated and remove » Consider CommentManagerInterface::addDefaultField() as deprecated and remove in favour of CommentTestTrait
Related issues: +#1847540: [META] Clean up comment module tests and decouple from node

@larowlan please help with CR, seems we need follow-up to clean/normalize

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

Draft CR https://www.drupal.org/node/2411239

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Reviewed & tested by the community

@larowlan Thanx a lot!
Looks ready for core

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Issue summary: View changes
Issue tags: -Needs change record +Needs reroll

Fixing the beta evaluation. Needs a reroll.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented

2357199-comment-trait-15.patch no longer applies.

error: core/modules/entity_reference/src/Tests/EntityReferenceSelectionAccessTest.php: No such file or directory
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Reviewed & tested by the community
FileSize
2357199-comment-trait-26.patch52.26 KB

re-roll after #2107243: Decouple entity reference selection plugins from field definitions

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 26: 2357199-comment-trait-26.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
2357199-comment-trait-26_.patch52.26 KB

re-roll

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community
pwieck’s picture

pwieck CreditAttribution: pwieck commented
Issue tags: -Needs reroll
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Yep this is part of ensuring the people use CMI properly. Committed 2d714fb and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Fixed » Reviewed & tested by the community

Was not commited, link points to bad commit object

  • alexpott committed 2d714fb on 8.0.x 
    Issue #2357199 by andypost, naiduharish: Consider...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed
yched’s picture

yched CreditAttribution: yched commented

Oh yeah :-)

Status: Fixed » Closed (fixed)

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