The patch in #2021779: Decouple shortcuts from menu links started a looong time ago and it was the reason for introducing ControllerBase/FormBase IIRC, but the patch wasn't updated to make use of the new shiny. Let's do it here.

CommentFileSizeAuthor
#20 2156923.19.patch9.59 KBjayeshanandani
#16 2156923.16.patch9.59 KBjayeshanandani
#14 2156923.8438230.patch9.81 KBmiraj9093
#14 interdiff.txt9.81 KBmiraj9093
#11 drupal_2156923_11.patch9.62 KBxano
#9 interdiff.txt3.02 KBamateescu
#9 2156923-9.patch9.83 KBamateescu
#6 drupal8-shortcut-2156923-6.patch8.76 KBinternetdevels
#1 2156923.patch8.74 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
Related issues: +#2021779: Decouple shortcuts from menu links
StatusFileSize
new 2156923.patch8.74 KB

This is what I found after looking at my own patch there with a very critic eye :)

Status: Needs review » Needs work

The last submitted patch, 1: 2156923.patch, failed testing.

amateescu’s picture

amateescu commented
Status: Needs work » Needs review

1: 2156923.patch queued for re-testing.

amateescu’s picture

amateescu commented

1: 2156923.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2156923.patch, failed testing.

internetdevels’s picture

internetdevels commented
Status: Needs work » Needs review
StatusFileSize
new drupal8-shortcut-2156923-6.patch8.76 KB

I've made it applicable.

amateescu’s picture

amateescu commented

Thanks @InternetDevels! Now if we could only find someone to review it.. :)

andypost’s picture

andypost
he/him
Primary language Russian
commented 
  1. +++ b/core/modules/shortcut/lib/Drupal/shortcut/Form/SetCustomize.php
@@ -17,30 +17,11 @@
+  protected $entity;
...
+   * The entity being used by this form.
...
+   * @var \Drupal\shortcut\ShortcutSetInterface

    Should be removed because defined in EntityFormController 

  2. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php
@@ -22,56 +16,11 @@
+   * The entity being used by this form.
...
+   * @var \Drupal\shortcut\ShortcutInterface
...
+  protected $entity;

    the same here 

  3. +++ b/core/modules/shortcut/lib/Drupal/shortcut/ShortcutFormController.php
@@ -83,7 +32,7 @@ public function form(array $form, array &$form_state) {
-      '#default_value' => $entity->title->value,
+      '#default_value' => $entity->getTitle(),

    Suppose better to use $this->entity here and remove previous $entity = $this->entity - the scope allows 

  4. --- a/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php
+++ b/core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php

@@ -10,7 +10,7 @@
- * Provides an interface defining a shortcut entity.
+ * Provides an interface defining a shortcut_set entity.

    s/shortcut_set/shortcut set

    The same should apply for ShortcutSet.php entity unit

amateescu’s picture

amateescu commented
StatusFileSize
new 2156923-9.patch9.83 KB
new interdiff.txt3.02 KB

Thanks for the review :)

1. and 2. I declared $entity again so we can have useful type hints in those classes.

3. and 4. Fixed.

andypost’s picture

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

Great! About 1 & 2 lets get commiter's feedback

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new drupal_2156923_11.patch9.62 KB

Re-roll, because the shortcut entity's link template was fixed already.

andypost’s picture

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

back to rtbc

alexpott’s picture

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

drupal_2156923_11.patch no longer applies.

error: patch failed: core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php:10
error: core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php: patch does not apply
miraj9093’s picture

miraj9093 commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new interdiff.txt9.81 KB
new 2156923.8438230.patch9.81 KB

Status: Needs review » Needs work

The last submitted patch, 14: 2156923.8438230.patch, failed testing.

jayeshanandani’s picture

jayeshanandani commented
Status: Needs work » Needs review
StatusFileSize
new 2156923.16.patch9.59 KB

Made changes and rerolled the patch.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Status: Needs review » Reviewed & tested by the community

#16 is a reroll of #11. I steped jayeshanandani through the process while also rerolling it myself. Our patches are identical, so RTBC.

alexpott’s picture

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

2156923.16.patch no longer applies.

error: patch failed: core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php:10
error: core/modules/shortcut/lib/Drupal/shortcut/Entity/Shortcut.php: patch does not apply
jayeshanandani’s picture

jayeshanandani commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 2156923.19.patch9.59 KB

Rerolled the patch. Merged without any conflicts.

jayeshanandani’s picture

jayeshanandani commented
StatusFileSize
new 2156923.19.patch9.59 KB

Just uploading the same patch since the testbot did not pick it up.

yesct’s picture

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

rtbc since it was rtbc'd by @ianthomas_uk and the latest reroll was an easy automatic one.
I also gave the patch a quick look and nothing jumped out at me to make it needs work.

alexpott’s picture

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

Committed 0f56bfa and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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