"More link" formatter configuration UI improvements

I looked at some alternative modules to see how they were laid out. The Smart Date Module and the Inline Entity Form module. I looked at the Widget Formatters for both and have attached some screenshots of each. I believe the drop down tab in the Image Formatter is the best option to achieve what your looking for. I have also attached a screenshot of the Image Formatter as well.

I moved all the "more" stuff into a HTML "Details" area.

Now that I look at the formatter config form a bit, I'm thinking there should also be a follow-up task to move the "Wrap trimmed content?" checkbox into "Additional Options". Perhaps "Additional options" should be a "Details" area as well?

-mike

There is now a merge conflict now.

setting to needs work... sorry

Rebased with some tweaks. Ready for review.

-mike

Moved a few things around for the details group, but this leads to another problem. The more_* settings are now more.more_*.. I was trying to find a way to override this, but ran out of time. I added a dummy saveForm() function but it's not part of the settingsForm() processing. I may have the wrong override name or some other stupid.. Short story long, with the group, the settings are no longer saving properly.

I am very hesitant to change the schema.

I did some work on this task during DrupalEasy office hours - it is not yet complete, and tests are failing (as expected).

It is clear that the schema will need to be updated, so I suppose an update hook will be in order.

Still work to do on this - no need for a review yet.

-mike

After Mike's Office Hours session, I've had a dig into this.

First of all, I can's see any way of adding a submit handler to a FormatterBase subclass. There is no submit method on the base class, and none of the implementations in core have one. So I don't think @markie's approach of using a saveForm function to move the values back to the existing schema will work.

I wanted to see what form values are getting submitted, and since I don's seem to be able to add a normal submit form, I added a temporary submit button:

    $element['submit'] = [
      '#type' => 'submit',
      '#submit' => [[$this, 'showSubmit']],
      '#value' => 'Show',
    ];

And handler function:

  public function showSubmit(array &$form, FormStateInterface $form_state) {
    $vals = $form_state->getValues();
    dpm($vals, "Submitted");
  }

This was helpful as it allowed me to determine that the values are in a "flat" structure by default, and it would seem that Drupal maps them to the schema:

array:13 [▼
  "trim_length" => "7"
  "trim_type" => "words"
  "trim_suffix" => ""
  "wrap_class" => "trimmed"
  "summary_handler" => "full"
  "more_link" => "1"
  "wrap_output" => 0
  "trim_options" => array:3 [▶]
  "more_class" => "more-link"
  "more_link_trim_only" => 0
  "more_target_blank" => 0
  "more_text" => "More"
  "more_aria_label" => "Read more about [node:title]"
]

I tried playing with the "#tree" property of the "more" element, to see if it would flatten the values, but I don't think it will allow us to do that, and if the fields are moved under the "more" element, then the schema has to match.

So while I agree with @markie, that changing the schema is undesirable, as far as I can see at present, it would appear to be unavoidable.

The one way I can think of to get around it would be to use JavaScript to move the form elements into the details, but I feel that would be hacky and more likely to cause problems later.

So, reluctantly, I agree with @ultimike that the schema change is unavoidable.

An install function to migrate existing setup to the new format should be straightforward enough. But does this require a change of major version?

Made a change to the schema to move "more_link" out of the "more" section, since it needs to be at the level above.

Tried to fix the tests, but still a couple of failing.

Still not ready for review.

I went down a rabbit hole of trying to make the "Details" section a visual grouping using the "#group" attribute. This would be attractive as it would allow us to avoid a schema change. However, this functionality doesn't seem to be fully implemented, and I haven't been able to get it to work at all on the Formatter form. There may be workarounds, but I don't think we should pursue a hacky solution.

This issue talks about making "#group" work properly, but doesn't seem to have progressed: 📌 Make #group FAPI / render feature work on all form/render #types out of the box Needs work

I'm considering whether the "more links" checkbox should be inside or outside the details group.

I've tried moving it inside to see how it looks, and I quite like it, as all the options are logically grouped.

Here it is when not checked:

And here it is checked, and the rest of the options shown:

I've moved the "display more link" checkbox back into the Details section as above. If people don't like it there I can move back out.

I've updated the schema and field names. I took "more_" out of the child names, as "more.more_link" feels redundant.

In the tests, because the "more" section is now an array, when updating the settings, we need to provide all members or excluded members will get overwritten. I've updated the tests accordingly.

All tests pass now.

I wouldn't consider this ready for review, as it needs the upgrade function. I'm giving that some thought, as I'm not sure how to test it. Ideally it would be nice to have some automated tests that set up the old schema, let the upgrade run, and verify it matches the new schema. Not sure if this is possible.

Changes look good so far - thanks, @lostcarpark!

I think there's a couple more related subtasks here (in addition to the update hook):

1. Add the 2nd-level "more" settings to the settings summary.
2. Add a functional test(s) for all the 2nd-level "more" settings (similar to the current functional tests like "more_link_trim_only").

As for the update hook, the hook itself shouldn't be too difficult - it's just reading and writing config. I looked at a bunch of contrib module code for modules that have update hooks related to configuration and I didn't find any that had a test for it. Granted, it wasn't an exhaustive search...

-mike

I've made some minor changes to the form, moving the "#states" value into a variable rather than repeating for each item.

I've attempted to use state to make the "more.text" field required if shown. This partially works, as the red * is shown, but it doesn't prevent a blank field from being saved.

There doesn't seem to be any way to validate the settings form. So at present it doesn't prevent a blank "more.text", or non-numeric values in the trim length. I tried using '#validate' to add a validation handler, but that didn't seem to work. Ideally, there should be server side validation on the submitted values, so hopefully there's a way to add some that I've overlooked. Failing that, JavaScript validation would be a (suboptimal) option.

I've also added "more link" details to the summary. Currently it could display: "More link enabled, text: Blah, only when trimmed, opens in new window". If that's too much detail, I could trim some of it.
I thought we should keep the more details to one line, but as translated text can't be concatenated, that meant having a translation string for each combination.

I'm wondering should display values in defaultSettings() be translated strings?

I've had a look through the formatters in core, and none of them seem to have translated values, but I don't think any of the defaults in core formatters are displayed, so I don't see a conclusive precedent.

I would have expected "More" and "Read more about [node:title]" to be translated.

I believe I have a strategy for automated testing of update script. Perform a minimal install in a dev site with the pre-update version of module, then save a dump of the database. The test script will restore the database, then apply updates and verify the outcome.
Here's an example: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/blo...

Thanks to @larowlan for point me at this.

Some minor changes to test cases.

Added checks for "only when trimmed" and "opens in new window" in summary (and checks they aren't present when option not selected).

Added new test to verify "More" link not present if not checked, and that it is present when checked.

@lostcarpark,

Regarding your comment 17 - does adding t() break the tests? If not, then let's add it.

Regarding your comment 18 - makes sense! Thanks for doing the legwork on that.

For the update hook, I would start with $storage = \Drupal::entityTypeManager()->getStorage('entity_view_display'); then maybe something like foreach ($storage->loadMultiple() as $view_display) { to loop around each one, looking for “our” configuration to update as you go…

-mike

Making good progress on update hook. Assigning to me while I work on it.

I have the update hook working. Working on a test for it.

I have created a test for the update script. Unfortunately it's failing on the `runUpdates()` call with the error "The link Continue was not found on the page."

This makes me suspect the issue is with my backup database.

I've pushed the test in case anyone has any ideas.

Found the problem by turning on browser output when running tests. Doh!

The problem was that my dev database is MySql, but Phpunit is running Sqlite, so the Sqlite module needs to be enabled before dumping the DB.

I also needed to specify the teaser view mode when loading to verify.

I've also added additional asserts to check old values removed.

Some quick thoughts after looking over the code so far:

1. Nice work with the $more_states and $more_required_states variables - much cleaner code now.
2. In the settingsSummary(), it may make more sense to output each "more" subfield individually, rather than the if-elseif stuff you have in there now (especially since not all subfields are represented).
3. Really nice work with SmartTrimUpdateMoreTest!
4. Is $this->drupalCreateNode() necessary in testMoreLink? What's wrong with the one in setup()?
5. Again, really nice work in hook_install()!

Thoughts?
-mike

Regarding 4, I don't think I wrote that test, but I removed the drupalCreateNode(), and it passes, so I agree it's not needed.

I also changed $this->assertNotEquals(); to $this->assertNull(); in that test, since the "target" attribute shouldn't be there at all if "open in new window" is not checked.

Have updated settingsSummary() with point 2 above. I was worried it might make it too long, but I think the previous version was too wide.
I've included all "More" fields on their own line.
If this is judged to be too long, one solution would be to only show the aria-label and class if they differ from the default.

Okay, I think it's time to set to Needs Review as I think the issue as raised is covered.

There are some things that I came across while looking at it, but I think they deserve to be dealt with in their own issues.

The fixtures directory contains the backup database of a minimal Drupal install in the "before update" state that the update test requires to run.

Here's an example of a similar test in core:

https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/blo...

And here's the fixtures directory it refers to:

https://git.drupalcode.org/project/drupal/-/tree/10.1.x/core/modules/sys...

I believe it's required for the test to run.

Yes, we need the fixture committed for the test. 100%

I tested manually and all of the formatter config appears to be updated to the new schema as expected (after running `drush updb`, of course).

All tests pass on my local as well.

Really nice work on the update hook (and test), @lostcarpark!

This is a significant change, so additional 👀 on this are welcome and most appreciated.

-mike

I had a slight misunderstanding. I had assumed the fixtures database dump was a .sql file, but I've checked the compressed file, and it's actually a .php file that creates the database with $connection->schema()->createTable statements. I've renamed the backup file to reflect this.

This is quite similar to the one in the one in the metatag project: https://git.drupalcode.org/project/metatag/-/tree/8.x-1.x/tests/fixtures

@lostcarpark - oh sweet!!! That's good to know, thanks for updating the PR. Does it still need to be compressed (.gz)?

The Metatag fixtures don't appear to be...

-mike

I've run the test with an upcompressed version of the database archive, and it runs correctly.

The UpdatePathTestBase base class handles the loading of the database. It seems to auto-detect if the file is compressed, and extract if required.

The main reason for compressing is the difference in the file size. The compressed version is 67KB, compared to 715KB for the uncompressed version. I imagine it gets compressed when uploaded to the GitLab repository, so it will only really make a difference on developer's local environments. So it's just a matter of whether we prefer to have easier access to the contents, or to save developers some space.

I'm happy to go with whatever the maintainers prefer.

If tests pass on d.o. with the compressed fixture, I say we go with that.

-mike

This is the update folder under the fixtures folder from #30:

https://git.drupalcode.org/project/drupal/-/tree/10.1.x/core/modules/sys...

There are a mixture of uncompressed and compressed files in the folder, but all of the most recent examples are compressed.

Great work everyone! Thanks for the ideas NotSoSolo! Now that it's not a database file I am happier with this whole thing. Tested locally and things look good. Merged!

Great stuff. It was never a database file, but I didn't understand that initially. It's good that you queried it, as the .sql extension on the original file was incorrect.

Great to see this one merged!

-mike

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

Thanks for the improvement! I have couple of issues:
1) When entity view display uses layout builder, than smart_trim_update_10201 function doesn't work, so need to manually re-save all such configs.
2) When saving config and then export it, there is `token_browser: ''` added to `more` section of config. This results in `missing schema` error from Configuration Inspector → module. So have to remove it manually and import config again.

Hi @ilya.no,

Thanks for reporting those. Would it be possible for you to create issues for them? It sounds like they are two separate issues, so I think a new issue for each would be appropriate.

I came here to say what @lostcarpark said. Please create new issues so we can review them.

Hello @lostcarpark and @markie
1) https://www.drupal.org/project/smart_trim/issues/3368700 📌 Latest version's smart_trim_update_10201() function doesn't work with layout_builder Needs work
2) https://www.drupal.org/project/smart_trim/issues/3368694 🐛 Remove token_browser from saved configuration of formatter Fixed
For second one I've created the patch, since it requires less work. For the first one, will try to work on it later.