Link to images styles from image field display settings

Patch adds link to formatter settings form.
Note this introduces a new string so can't be backported.
The existing string is 'Image styles' so I don't think that provides enough context. Happy to swap it out for something existing if it can be found.

That patch worked beautifully for me, minus a Git warning: "1 line adds whitespace errors" (looks like line 9).

Updated patch without the whitespace that was causing the error.

That patch seems OK. However, when I want to go back to field display settings page, I have to click twice on the "Back" button of my browser. Clicking once should be enough.

Would that be an Overlay issue?
I tested the patch and SuperHeron's comments on the click back and it is confusing but not sure if this patch can "fix" that?
Plus isn't the back button suppose to go away? ( -:

Patch from #5 works perfectly.
"Go back" button also works if Overlay is disabled.

+  $item = menu_get_item('admin/config/media/image-styles');
+  if ($item['access']) {

What's wrong with user_access()? Also for altering it's generally better to assign #access to the form element rather than conditionally defining it.

Nice patch! I think we don't use the 'manage' word that much, probably more consistent to say 'Configure image styles'.

i changed the word manage to configure as mentioned in 10

#11: 1433796-manage-image-styles-10.patch queued for re-testing.

Please find the attached patch as per comment #9 and #10.

Thanks.

#15: 1433796-configure-image-styles-10.patch queued for re-testing.

I am going to try to rework this patch for the latest version of d8.

Patch File attached.

18: 1433796-configure-image-styles-11.patch queued for re-testing.

I changed the capitalization of the link while also translating it. Also, converted it to use dependency injection. All of this during the Austin sprint.

Patch looks like it works. Link is where it's expected an takes user to expected destination. Screen shot attached.
Changed status to RTBC

Got out of sync with #27. Will review again.

Updated as Drupal 8 Apis changed.
Please review now.

Thank you.

1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -56,7 +56,9 @@ public function settingsForm(array $form, array &$form_state) {
   +    $element['configure_styles'] = array(
   +      '#markup' => \Drupal::l($this->t('Configure Image Styles'), 'image.style_list'),
   +    );
   

   Don't you think that this needs to go just below the "Image style" select as it's referring to that element?

2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -56,7 +56,9 @@ public function settingsForm(array $form, array &$form_state) {
        return $element;
   

   Please add an empty line before return statement. Also between each form element as is now.

updated patch as #31. Please review now.

Thank you.

1. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -44,11 +44,16 @@ public function settingsForm(array $form, array &$form_state) {
   +    ¶
   ...
   +    ¶
   
   @@ -56,7 +61,7 @@ public function settingsForm(array $form, array &$form_state) {
   +    ¶
   

   Trailing spaces at the end of lines. Remove them.

2. +++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
   @@ -44,11 +44,16 @@ public function settingsForm(array $form, array &$form_state) {
        $link_types = array(
          'content' => t('Content'),
          'file' => t('File'),
        );
   

   There's no need for empty line here while $link_types is together with the next element.

Also please publish every time an interdiff, otherwise is hard to track changes.

Updated patch as #33

Good to go for me.

We seem to have lost the dependency injection fro #27 why?

Also shouldn't we only be adding the link if the user has the administer image styles permission?

Added back the dependency injection but cleaned up and re-adapted the constructor signature.

I agree with #37, 2nd remark: add an #access property to the form item.

And a small doc error:

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -20,7 +24,55 @@
+   * Constructs a FormatterBase object.
+   *

Constructs an ImageFormatter object.

Ah, I missed that from #37.

I'm fine with the patch now. However, I see that e.g. CommentForm (core\modules\comment\src\CommentForm.php) injects the current user (whereas NodeForm (core\modules\node\src\NodeForm.php) does not. As we tend to use injection in OO code and try to reserve Drupal:: for procedural code only, I think it would be just a bit better to inject the current user here as well.

What do you think?

Ha, ha :) I only checked NodeForm and decided to go in that way. But I agree that injection is better.

Will rework the patch.

Complete the documentation for the constructor by adding:

   * @param \Drupal\Core\Session\AccountInterface $current_user
   *   The current user account.

After that the patch is OK.

(Note: PhpStorm warns you for these omissions, you may apply for a free license for open source development)

And please add an interdiff.txt against #42.

Besides reviewing, I also tested locally that the access check indeed works: RTBC

We should be testing this - otherwise we could potentially break this.

Trying to cover this in tests.

Thanks for adding a test. A few questions arise though:
- Should we use assertLink(), assertLinkByHref() (my preference) or both?
- Should we test the negative case of the #access part (I think so)?

Leaving to NR to get more opinions on this, but feel free to make a new patch conforming to your (or mine) view on this.

Taking at look at this at NERDSummit2014

Taking over from eporama

Rerolled patch from #53 against 8.0.x

My remarks from #54 are still standing. with those 2 changes I will RTBC.

I still very much like this patch. Adding a fresh screengrab, the one in #48 shows it in a different place than it is now:

I think it would be even better if we put the link next to the select list. It makes this link slightly less prominent by taking it out of it's own line, removing it from the top to bottom flow. Slightly less prominent is good here because you won't need this link most of the time. It's not the same as the link for editing the machine name of things, but ginving this link the same amount of prominence here seems right:

Can somebody reply to the questions in #54?

#63: Good idea, seems like a nice improvement to me.
- Perhaps with brackets around, or is that not international enough, i.e. do brackets have to be translated and is that worth the hassle?
- If it is done in css, assure that RTL languages keep working.
- Open in new tab or do we never do that? I'm normally opposed to this as the user can force this him/herself, but in this case the user may loose (a lot of) changes by (unintentionally) clicking on this link. OTOH, any new styles won't show when returning to this tab.

- I don't the brackets are necessary here, I think they were added to the machine name pattern to illustrate that this value is a "synonym" for the value in the text field before it.
- yup
- We never force a link to open in a new tab, but you bring up an important point. That any new image styles won't show up is enough reason to not open in a new tab, but we should also prevent losing unsaved changes. AFAIK that risk is already there (you'll lose changes to individual field settings if you don't save on the whole content type) and I think there's already an issue open about that. Maybe this is a case where you'd want the image styles ui to show up in a modal window, but then still only if a new image style will become available once you exit that modal. Hmmm.

I am not sure if we should worry about it too much. Unlike other settings, clicking away here - although destructive is only a minor inconvenience. As its likely 3/4 settings.

Ideally we have some form of localstorage here, just like we do with views. But realistically we can probably just get away with the fact that people lose data, since its relatively easy to recover. I am not sure if it makes much sense to introduce a one-off pattern, for that inconvenience.

Thanks, that sounds like the right trade off to me as well.

I agree, as well, I just wanted to raise the options. So, a new patch should add #63 and #54 and it's OK.

The patch from #60 was causing a PHP error when I browsed to /admin/structure/types/manage/article/display: it looks like the second parameter of LinkGeneratorInterface::generate() had been changed from a string to a URL type since then. I fixed that and also modified the first parameter based on the recommendation for translatable text given in LinkGeneratorInterface::generate()'s documentation.

I've incorporated the recommendations from #54 as follows:
- Changed the assertLink() test to use assertLinkByHref() instead
- Added a test to check that the link isn't present if the user lacks 'administer image styles' permission

I'm not sure if the code I added for the additional test is the cleanest way of doing it, so if you have feedback on that I'd appreciate it.

I wasn't sure how to cleanly implement the recommendation from #63 to move the link to the right side of the UI, so that is not done yet.

Thanks for updating the patch.

Implementing #63 does not seem so straightforward as there is inline text before the element, so just playing with display and/or float did not give me the desired results. To implement #63, we could add the link as the #description of the select element. this will place the link within the form element wrapper:

if ($this->currentUser->hasPermission('administer image styles')) {
  $element['image_style']['#description'] = $this->linkGenerator->generate($this->t('Configure Image Styles', array('@url' => \Drupal::url('image.style_list'))), \Drupal\Core\Url::fromRoute('image.style_list'));
}

From there on, we need to make this description inline-block and give it some space before (I guess that putting a space in the :before will work for both LTR and RTL, whereas a margin-left will only work for LTR). so we need to add some css to it.

Rest of the patch is RTBC, thus if we don't consider #63 worth the hassle, the whole patch is RTBC for me.

Putting the link into #description was a big help; thanks @fietserwin!

The link now appears to the side of the main UI flow, as per #63. I implemented RTL support based on how I've seen it done elsewhere in D8, using media queries.

To have the description wrap on very small screens, I arrived at this css. it also includes my idea for RTL support, feel free to pick your preferred method, but I think that overruling the nowrap should be added:

#field-display-overview .form-item-fields-field-image-settings-edit-form-settings-image-style {
  white-space: normal;
}
#field-display-overview .form-item-fields-field-image-settings-edit-form-settings-image-style .description {
  display: inline-block;
}
#field-display-overview .form-item-fields-field-image-settings-edit-form-settings-image-style .description:before {
  content: "\00a0";
}

Note: \00a0 is the Unicode code for a nonbreakable space, as   does not work in content, see e.g. http://stackoverflow.com/questions/16552397/css-how-to-add-white-space-before-elements-content.

I added the CSS to override nowrap. I've left the implementation of RTL support as-is for consistency with other D8 CSS, though I do think your suggested way of doing it is pretty slick :)

Thanks, everything looks good now.

I was surprised by the possibilities of the #description tag you used (not a simple string, but a render array). It is not documented on the Form API page, but it works. However, the description div gets rendered but remains empty, only the content (the render array) is not rendered. And the aria-describedby attribute on the select element thus refers to an empty element or to the link.

+++ b/core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
@@ -44,8 +108,11 @@ public function settingsForm(array $form, FormStateInterface $form_state) {
+        '#markup' => $this->linkGenerator->generate($this->t('Configure Image Styles', array('@url' => \Drupal::url('image.style_list'))), \Drupal\Core\Url::fromRoute('image.style_list')),

We should be injecting the url_generator here instead of using \Drupal::url()

Thanks for picking this up. I think that this addresses the concern of #75.

This issue is a prioritized change (usability) as per https://www.drupal.org/core/beta-changes and it's benefits outweigh any disruption. Committed 3741a5e and pushed to 8.0.x. Thanks!