Credits should go also to #1809376: Use EntityListController for image styles issue contributors: andypost, speely, fubhy, heyrocker.
Image styles are the config objects now.
It would be great example of working with complex config entities along with #1588422: Convert contact categories to configuration system
Suppose this issue should wait for #606598: Human readable image-style names
Postponed on #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) and #1809376: Use EntityListController for image styles.
Related issues
#1821848: Move image style load/update/delete operations into a new ImageStyleStorageController
Followups
Comment | File | Size | Author |
---|---|---|---|
#84 | 1788542-image-form-84.patch | 36.43 KB | claudiu.cristea |
#84 | interdiff.txt | 29.88 KB | claudiu.cristea |
#80 | 1788542-image-form-80.patch | 36.42 KB | claudiu.cristea |
#80 | interdiff.txt | 23.6 KB | claudiu.cristea |
#78 | 1788542-image-form-78.patch | 37.81 KB | claudiu.cristea |
Comments
Comment #1
larowlanSee also #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity)
Comment #2
larowlantagging
Comment #3
gddPostponed until #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity) lands
Comment #4
gddI'm just going to roll this into #1782244: Convert image styles $style array into ImageStyle (extends ConfigEntity). It doesn't make sense to have two separate issues.
Comment #5
andypostRight status
Comment #6
gddAccording to the discussion at #1588422-42: Convert contact categories to configuration system we are actually tackling these in separate issues, so reopening this.
Comment #7
andypost#1588422: Convert contact categories to configuration system commited
Comment #7.0
andypostUpdated issue summary.
Comment #8
andypostthere's a big changes in hook_menu for image module in related #1809376: Use EntityListController for image styles
So I'd recommend to follow current core's pattern entity-type/manage/%id/action for menu
Comment #9
tim.plunkettSee also: #1885644: ImageStyle::uri() is broken and has no coverage
Comment #10
tim-e CreditAttribution: tim-e commentedComment #11
tim-e CreditAttribution: tim-e commentedIve had a run at moving the create new style form to entity form controller.
In discussion with larowlan and heyrocker, agreed just to do that one form as the effects will be converted to plugins as per http://drupal.org/node/1821854
Comment #12
sunHm. What about the edit form though?
Comment #13
tim.plunkettAwesome start!
You can use $entity = $this->getEntity(); and then call $entity->id()/label() here
This should probably have
'#disabled' => !$entity->isNew(),
You should be able to use $this->getEntity() here
You should call parent::actions() and just override the #value of the button name, since you definitely want the EntityFormController::submit() function to run.
Comment #14
tim-e CreditAttribution: tim-e commentedAttached is a new patch taking into consideration the above feedback and implementing the edit form as well.
I would like to get some feedback on this as it may need some work.
Also, I'm not quite sure what to do about the Delete form. Should I be trying to make the form() method handle all three operations add/edit/delete? In this patch it only handles add/edit and the delete form is still using drupal_get_form. I was thinking pass along the url parameter add/edit/delete as an $operation variable to the form() method and could base it on that. But maybe there is a better way?
Comment #15
tim.plunkettNone of the conversions have dealt with delete, mostly because of the confirm form. So I wouldn't worry about converting that here either.
I didn't have a chance to review this yet, but thanks tim-e for keeping this moving!
Comment #17
andypostweights needs bigger step and should apply to all elements
Suppose this require follow-up issue to convert this theme function to new #table element
set() is not needed here
last redirect will override one for effects
Comment #18
tim.plunkettWe should always be using Entity::set() for that, even public properties. So keep that part. The other feedback looks valid.
Comment #19
andypost@tim I mean that values are already set in
parent::submit()
so$this->getEntity($form_state);
will return a form values and all you need just to call$entity->save()
Comment #20
tim-e CreditAttribution: tim-e commentedHave had another run at this. May still need some work but should resolve the above feedback.
Comment #21
tim-e CreditAttribution: tim-e commentedIgnore the last patch, will fix and repost
Comment #22
tim-e CreditAttribution: tim-e commentedTrying again
Comment #24
tim.plunkettTrailing whitespace.
These are all opposite changes. Should be reverted.
Contains \Drupal\...
Extra blank line
-----
I couldn't spot any reason for the failures, but if the rest of this is cleaned up first, I'll make another pass at it.
Comment #25
tim-e CreditAttribution: tim-e commentedAnother run at this. All tests green for me locally.
Comment #26
larowlanThis is looking awesome.
Some minor nitpicks.
There is no 5th argument here, the callback has default to NULL so you don't need this line.
New standard is \Drupal
Same
The extra blank lines from comment #24 are still present.
Leaving at needs review for others.
Comment #27
tim-e CreditAttribution: tim-e commentedOk fixed up those issues as noted by larowlan
Comment #28
tim-e CreditAttribution: tim-e commentedConverting to controller/route
Comment #29
andypost#27: 1788542-image-style-entity-form-controller.patch queued for re-testing.
Comment #30
andypost@tim-e I think route conversion should be done in follow-up
Comment #31
andypostoptional is not a data type
_form is a bad suffix for page callback
Please make different callback for edit page to make proper page title. see contact module as example
Comment #32
andypostAdds more fixes
Comment #33
andypostRe-roll after #1807042: Reorganizie entity storage/list/form/render controller annotation keys
Comment #34
andypostRe-roll with routes and new EntityFormController
Comment #35
andypostrevert drupal_set_title()
EDIT or we need to use $this->operation like #1938386-23: Convert contact_category_* pages to a new-style Controller does
Comment #37
andypost#35: 1788542-image-form-35.patch queued for re-testing.
Comment #38
kim.pepper#35: 1788542-image-form-35.patch queued for re-testing.
Comment #39
podarok#35 looks good for me
happy pushing
Comment #40
podaroktagging
Comment #41
catch#35: 1788542-image-form-35.patch queued for re-testing.
Comment #43
alexpottI'm also not sure about using the same form controller for create and edit as the forms are so different.
Comment #44
tim.plunkettPlease don't reuse the same operation.
See #2006348: Remove default/fallback entity form operation
At the least it should be
But ideally they'd be different classes, maybe with a base class.
Comment #45
andypostjust a re-roll
Comment #47
andypostNew patch with separated forms (as it was)
Comment #49
andypost#47: 1788542-image-form-47.patch queued for re-testing.
Comment #51
andypostdescription is not used for action links
List controller should be commited first #1809376: Use EntityListController for image styles
then
interdiff-todo.txt
should be applied for #1908756: Separate Action Links (MENU_LOCAL_ACTION) from hook_menu()EDIT test passes locally
Comment #52
andypostSo finally should be fine
Comment #53
tim.plunkettBlocks removal of MENU_LOCAL_ACTION
Comment #54
dawehner#52: 1788542-image-form-52.patch queued for re-testing.
Comment #55
andypost#52: 1788542-image-form-52.patch queued for re-testing.
Comment #56
claudiu.cristeaThis must be broken now after committing #1821854: Convert image effects into plugins, #2027423: Make image style system flexible and #1987712: Convert file_download() to a new style controller. I'll rework it on top of them.
Comment #57
claudiu.cristeaComment #58
claudiu.cristea#1809376: Use EntityListController for image styles was merged here because they interfere in a way that make difficult separate patching.
Here's a reworked patch, including #1809376: Use EntityListController for image styles.
KNOWN BUG: The label/title of the local action Add image style is missed from markup. I followed exactly https://drupal.org/node/2007444 for conversion. Here I need some help.
An
interdiff.txt
was impossible to provide. Image tests ran successfully local.Comment #59
tim.plunkettThis is one directory too low. Should be
namespace Drupal\image\Plugin\Menu\LocalAction;
Comment #62
claudiu.cristeaComment #63
claudiu.cristeaComment #64
dawehnerNo need for the initial backslash.
There should be an empty line before the end.
Theoretical you can use array($storage_controller, 'load')
Is there a follow up to convert this to a library?
Are you sure we need this? theme itself should load the file on the fly.
It would be nice to inject this service ... instead
t() should be also injected
Unnecessary whitespace left.
Let's put symfony to the bottom, as it looks nicer.
Comment #65
fubhy CreditAttribution: fubhy commentedDue to the merge with #1809376: Use EntityListController for image styles this patch should also give commit credit for "andypost, fubhy, speely | heyrocker".
Comment #65.0
fubhy CreditAttribution: fubhy commentedUpdated issue summary.
Comment #65.1
claudiu.cristeaUpdated issue summary.
Comment #66
claudiu.cristeaIssue summary changes:
Comment #67
claudiu.cristeaHere we go...
Comment #69
claudiu.cristeaThis passed Image tests locally.
Comment #69.0
claudiu.cristeaUpdated issue summary.
Comment #70
claudiu.cristeaMore cleanup.
Comment #71
claudiu.cristeaAdded call to controller's method instead procedural call to
image_style_load
in$form['name']['#machine_name']['exists']
inImageStyleAddFormController
too.Comment #72
fubhy CreditAttribution: fubhy commentedYou are only ever using the storage controller. So let's just inject the storage controller, not the entire entity manager.
Looks good otherwise.
Comment #73
claudiu.cristeaThank you!
OK, then let's change
ImageStyleAddFormController
too :)Comment #74
andypostSuppose imageStyleStorageController is better. We have this all over core
Comment #75
claudiu.cristeaFrom IRC:
Comment #76
claudiu.cristeaYeah! It's green. Any other review or rtbc?
Comment #77
fubhy CreditAttribution: fubhy commentedMisses leading backslash.
Misses period.
But that's really it. Just read the code twice.
Comment #78
claudiu.cristeaOK, thank you. It should pass -- only docs.
Comment #79
tim.plunkettThere SHOULD be an
abstract class ImageStyleFormBase
that both the add and edit extend. There is a lot of code duplication here.Because of this trickiness using $entity_type in createInstance(), you have absolutely NO idea what storage controller this is.
Please rename $storageController to $imageStyleStorage, and $storage_controller $image_style_storage
Just pass $module_handler to parent::__construct() first, no need to do the assignment ourselves.
This needs to be
return parent::form($form, $form_state);
at the end, stuff in the parent expects to run laterThis should be removed, it wasn't in the original code.
I guess this works, I haven't seen it because we didn't have singular load() available before. But most other forms use entityQuery for this since we don't need it loaded
This should be removed, there is no need to add watchdog calls.
A lot of my comments about the previous class apply to this as well.
Please don't introduce this pattern. Use $this->entity directly.
Don't bother, just use $this->entity please
Updates
Say which entity
What's this for? The parent does this.
Isn't there an existing web test this could go in?
This can be a oneliner
$this->drupalLogin($this->drupalCreateUser(
, no need for a propertypublic
Can you assert that the correct page loads?
Comment #80
claudiu.cristeaVoilà!
Comment #81
tim.plunkettWow @claudiu.cristea, that was fast.
You can drop the Controller suffix if you'd like, to match the base class.
This should be ImageStyleEditForm
I meant, why are you overriding $row['label'] at all? Just delete this line.
public function test
Comment #82
claudiu.cristeaYes, I was thinking to rename them. Woudn't be a good idea to rename an also move them in the Form subdirectory?
There is an existing Form subdir holding delete form.
Comment #83
tim.plunkettYes please
Comment #84
claudiu.cristeaOK.
Comment #86
claudiu.cristeaHmm...
Drupal\system\Tests\Module\ModuleApiTest
is passing locally with latest code pulled.Comment #87
claudiu.cristea#84: 1788542-image-form-84.patch queued for re-testing.
Comment #88
tim.plunkettLooks good to me, thanks @claudiu.cristea!
Comment #89
claudiu.cristeaRemove some tags.
Comment #90
alexpottPatch works fine but image effect ordering is displaying some interesting behaviour in HEAD that this patch neither fixes or changes, see #2052787: Image style effect ordering exhibits some odd behaviour.
Committed 5f317ad and pushed to 8.x. Thanks!
Comment #91
Gábor HojtsyYay, this will let image effect translatability be exposed on the UI! Woot!
Comment #92.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #93
sunIn #79, @tim.plunkett stated:
I'd like to learn and understand why that advice was given — are user operations logged differently in D8 now?
Drupal did log a message for each (form/UI driven) entity operation in the past, at minimum for create/update/delete operations.
Not limited to administrative configuration changes, but in particular for those, the logging is related to security (security audits).
In fact, Drupal did not always log all user operations in the past — typically, modules simply forgot to implement it, causing an incomplete picture in security [breach] audits.
Unless the logging is (centrally?) handled elsewhere now, this removal appears to be a step backwards and a regression in terms of security to me.
A clarification would be highly appreciated. :)
Comment #94
larowlanYeah, I can't see any central handling of this either, but perhaps we should open an issue to add it?
Having it handled centrally would harden contrib modules too, one less thing for them to think about.