Problem/Motivation

Noticed when reviewing #3418442: Revisions should be filtered by language in the database query

In RevisionOverviewForm::buildForm we have some logic to get the previous revision and pass it to ::buildRevision which then passes it on to $this->entityComparison->getRevisionDescription (another unecessarily public method).

getRevisionDescription does nothing with $previous_revision so all of this is a waste.

Proposed resolution

Remove handling from buildForm
Remove param from buildRevision and pass NULL to getRevisionDescription
Potentially deprecate getRevisionDescription on the entityComparison service since it's only called from buildRevision and move it into the Form class. We can then remove the entityComprison service from the form as well.

Remaining tasks

User interface changes

N/A

API changes

See above

Data model changes

Issue fork diff-3568877

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

acbramley created an issue. See original summary.

yusuf_khan’s picture

yusuf_khan commented
Assigned: Unassigned » yusuf_khan

@acbramley - Im Looking into this issue

yusuf_khan’s picture

yusuf_khan commented
Assigned: yusuf_khan » Unassigned
Status: Active » Needs review

Please review the MR - https://git.drupalcode.org/project/diff/-/merge_requests/152

yusuf_khan opened merge request !152

acbramley’s picture

acbramley commented
Status: Needs review » Needs work

The parameters need to be removed from the methods. Please see the proposed resolution in the issue summary.

acbramley closed merge request !152

acbramley opened merge request !153

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

joseph.olstad made their first commit to this issue’s fork.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Assigned: Unassigned » joseph.olstad
Status: Needs review » Active
acbramley’s picture

acbramley commented
Status: Active » Needs work
joseph.olstad’s picture

joseph.olstad
Primary language French
commented

11.4 next minor tests are failing. ah, I see, it's fixed in 2.x, merging 2.x into the MR

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Assigned: joseph.olstad » Unassigned
Status: Needs work » Needs review
joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Merging this will result in large conflicts with 2452523. If it was me calling the shots, I'd merge 2452523 first and then redo this merge request based on that instead.

I recommend postponing on #2452523: Offer a revisions tab for all entities , it's been 11 years now, everything else can wait IMHO!

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs review » Postponed

Postponed on #2452523: Offer a revisions tab for all entities

joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Related issues: +#2452523: Offer a revisions tab for all entities

This issue is in direct conflict with 2452523. Adding related issue link.

acbramley’s picture

acbramley commented
Status: Postponed » Needs work

It is not postponed on this issue. The conflict is exactly why we're getting this one in first.

  • acbramley committed fd5eb9e9 on 2.x 
    task: #3568877 Remove $previous_revision handling from...
joseph.olstad’s picture

joseph.olstad
Primary language French
commented
Status: Needs work » Fixed

marking this as fixed

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.

yusuf_khan’s picture

yusuf_khan commented

@maintainer can you please givr me contibution credit.

Status: Fixed » Closed (fixed)

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

danflanagan8’s picture

danflanagan8
Primary language English
Location St. Louis, US
commented

This was a pretty unpleasant surprise when I just updated from beta5 and discovered that the public method getRevisionDescription had disappeared from the diff.entity_comparison service. Luckily I have good test coverage for my project code that integrates with diff.

I know it's technically beta but I'm pretty sure that even then it's poor form to remove a public method from a service.