Closed (fixed)
Project:
Drupal core
Version:
8.0.x-dev
Component:
shortcut.module
Priority:
Normal
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
15 Dec 2013 at 10:34 UTC
Updated:
29 Jul 2014 at 23:13 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
amateescu commentedThis is what I found after looking at my own patch there with a very critic eye :)
Comment #3
amateescu commented1: 2156923.patch queued for re-testing.
Comment #4
amateescu commented1: 2156923.patch queued for re-testing.
Comment #6
internetdevels commentedI've made it applicable.
Comment #7
amateescu commentedThanks @InternetDevels! Now if we could only find someone to review it.. :)
Comment #8
andypostShould be removed because defined in EntityFormController
the same here
Suppose better to use $this->entity here and remove previous $entity = $this->entity - the scope allows
s/shortcut_set/shortcut set
The same should apply for ShortcutSet.php entity unit
Comment #9
amateescu commentedThanks for the review :)
1. and 2. I declared $entity again so we can have useful type hints in those classes.
3. and 4. Fixed.
Comment #10
andypostGreat! About 1 & 2 lets get commiter's feedback
Comment #11
xanoRe-roll, because the shortcut entity's link template was fixed already.
Comment #12
andypostback to rtbc
Comment #13
alexpottdrupal_2156923_11.patch no longer applies.
Comment #14
miraj9093 commentedComment #16
jayeshanandani commentedMade changes and rerolled the patch.
Comment #17
ianthomas_uk#16 is a reroll of #11. I steped jayeshanandani through the process while also rerolling it myself. Our patches are identical, so RTBC.
Comment #18
alexpott2156923.16.patch no longer applies.
Comment #19
jayeshanandani commentedRerolled the patch. Merged without any conflicts.
Comment #20
jayeshanandani commentedJust uploading the same patch since the testbot did not pick it up.
Comment #21
yesct commentedrtbc 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.
Comment #22
alexpottCommitted 0f56bfa and pushed to 8.x. Thanks!