Problem/Motivation

I don't like this key shifting for +1. And then i realised in RevisionOverviewForm, we are loading count($vids)-1 twice because we just pass in the previous revision id and then load it deep inside summary(). You try to delegate loading, but still need to maintain conditions if $key isn't existing. That's bad delegation. If it's the last revision, just don't pass in a second revision(_id) at all. But i think we should switch $previous_revision_id with $previous_revision and load early. Small API change although beta, but IMHO mostly internal.

Improve getRevisionDescription to avoid this bad delegation.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#9 interdiff-2804021-7-9.txt3.77 KBtbonomelli
#9 improve_getrevisiondescription-2804021-9.patch6.05 KBtbonomelli
#7 interdiff-2804021-4-7.txt1.57 KBtbonomelli
#7 improve_getrevisiondescription-2804021-7.patch5.19 KBtbonomelli
#4 improve_getrevisiondescription-2804021-4.patch5.18 KBtbonomelli

Comments

yongt9412 created an issue. See original summary.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Priority: Normal » Critical

Critical because API change.

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented
Related issues: +#2802417: Remove revision log in revision listing from Revision column

Added related issue.

tbonomelli’s picture

tbonomelli commented
Assigned: Unassigned » tbonomelli
Status: Active » Needs review
StatusFileSize
new improve_getrevisiondescription-2804021-4.patch5.18 KB

Loaded the previous_revision early and passed the previous_revision instead of the previous_revision_id.

toncic’s picture

toncic commented
Status: Needs review » Needs work

Looks good for me just little space.

+++ b/src/Form/RevisionOverviewForm.php
@@ -189,6 +189,12 @@ class RevisionOverviewForm extends FormBase {
+      if(isset($vids[$key + 1])) {

Space between if and '(', .

johnchque’s picture

johnchque
Primary language Spanish
Location Bolivia
commented 
+++ b/src/DiffEntityComparison.php
@@ -261,21 +261,21 @@ class DiffEntityComparison {
+  public function getRevisionDescription($revision, $previous_revision) {

we might want to set $previous_revisions as optional?

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new improve_getrevisiondescription-2804021-7.patch5.19 KB
new interdiff-2804021-4-7.txt1.57 KB

Made $previous_revision optional in both getRevisionDescription() and summary().
Also removed an unneeded space.

tduong’s picture

tduong commented
Status: Needs review » Needs work

More feedbacks :P

  1. +++ b/src/DiffEntityComparison.php
@@ -261,21 +261,21 @@ class DiffEntityComparison {
+   * @param $previous_revision

@@ -285,18 +285,15 @@ class DiffEntityComparison {
    * @param $revision
...
+   * @param $previous_revision

    Add \Drupal\Core\Entity\ContentEntityInterface as @param type. 

  2. +++ b/src/DiffEntityComparison.php
@@ -267,7 +267,7 @@ class DiffEntityComparison {
+  public function getRevisionDescription($revision, $previous_revision = NULL) {

@@ -291,9 +291,9 @@ class DiffEntityComparison {
+  protected function summary($revision, $previous_revision = NULL) {

    If you add NULL, then you need to update the docblock as well, specifying that this param is optional and defaults to NULL. 

  3. +++ b/src/DiffEntityComparison.php
@@ -261,21 +261,21 @@ class DiffEntityComparison {
+  public function getRevisionDescription($revision, $previous_revision) {

@@ -285,18 +285,15 @@ class DiffEntityComparison {
+  protected function summary($revision, $previous_revision) {

    For both params, add ContentEntityInterface as type hints. 

  4. +++ b/src/Form/RevisionOverviewForm.php
@@ -189,6 +189,12 @@ class RevisionOverviewForm extends FormBase {
+        $previous_revision = NULL;

    You need to declare $previous_revision = NULL; before the if check, then you can remove the else block. 

  5. +++ b/src/Form/RevisionOverviewForm.php
@@ -321,13 +325,13 @@ class RevisionOverviewForm extends FormBase {
    * @param $revision
...
+   * @param $previous_revision

    Add same param type mentioned above. 

  6. +++ b/src/Form/RevisionOverviewForm.php
@@ -321,13 +325,13 @@ class RevisionOverviewForm extends FormBase {
+  protected function buildRevision($link, $username, $revision, $previous_revision) {

    Add the type hints like above.

tbonomelli’s picture

tbonomelli commented
Status: Needs work » Needs review
StatusFileSize
new improve_getrevisiondescription-2804021-9.patch6.05 KB
new interdiff-2804021-7-9.txt3.77 KB

Added the @Param types and type hints. Also changed $previous_revision to optional in function buildRevision().
Removed the else block and declared $previous_revision = NULL before the if statement.

tduong’s picture

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

Looks fine :)

miro_dietiker’s picture

miro_dietiker
Location Switzerland
commented
Status: Reviewed & tested by the community » Fixed

Yes, i like. Committed.

Status: Fixed » Closed (fixed)

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