Problem/Motivation

shortcut insert/update produce always the same message; Looking at ShortcutForm::save()

  public function save(array $form, FormStateInterface $form_state) {
    $entity = $this->entity;
    $entity->save();

    if ($entity->isNew()) {
      $message = $this->t('The shortcut %link has been updated.', array('%link' => $entity->getTitle()));
    }
    else {
      $message = $this->t('Added a shortcut for %title.', array('%title' => $entity->getTitle()));
    }
    drupal_set_message($message);
....
  }
  • The conditions are reversed, then you should always see the wrong message.
  • $entity->isNew() is called after $entity->save() so the entity'll never be new.

Proposed resolution

Show correct message on shortcut insert/update

User interface changes

none.

API changes

none.

CommentFileSizeAuthor
#5 d8-shortcut-save-wrong-message-2469667-5.patch2.31 KBm4olivei
#1 d8-shortcut-save-wrong-message-2469667-1.patch650 byteswillzyx

Comments

willzyx’s picture

willzyx commented
StatusFileSize
new d8-shortcut-save-wrong-message-2469667-1.patch650 bytes
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Looks great, tested on latest D8, patch applied clean and works as advertised. The update message shows on update now, the new message shows only when new.

@willzyx++

xano’s picture

xano
he/they
Primary language English
Location Southampton
commented
Status: Needs review » Reviewed & tested by the community

Nice catch! Thanks :)

alexpott’s picture

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

Looks like we sh/could add an assertion somewhere for this.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Needs work » Needs review
StatusFileSize
new d8-shortcut-save-wrong-message-2469667-5.patch2.31 KB

Sounds reasonable @alexpott. Patch attached with a couple assertions addd to look for the right message. What do you think? Totally open to critique here, I'm climbing the Drupal Ladder, and this seemed like a nice opportunity to learn some things and submit a patch.

willzyx’s picture

willzyx commented 
+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -64,6 +64,7 @@ public function testShortcutLinkAdd() {
       $this->drupalPostForm('admin/config/user-interface/shortcut/manage/' . $set->id() . '/add-link', $form_data, t('Save'));
       $this->assertResponse(200);
+      $this->assertText(t('Added a shortcut for @title.', array('@title' => $title)));
       $saved_set = ShortcutSet::load($set->id());

I do not know if this test inside a loop is a good thing. Maybe we can create a dedicated shortcut just to test the message

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

That seems fair, although there are a bunch of other assertions in that loop already, doesn't seem much harm to have this in there as well. Were you thinking to create a new test case for this, to just test the message? The testShortcutLinkAdd method is all about operating on a list of links to create.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Priority: Minor » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

@willzyx I think it's not an issue. This is good to go thanks for working on this @m4olivei and @willzyx.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Ha, nice catch. That was totally wrong. :)

Looks like we have test coverage for this now too, so...

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 0711885 on 8.0.x 
    Issue #2469667 by willzyx, m4olivei: Wrong message on shortcut insert/...

Status: Fixed » Closed (fixed)

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