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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willzyx’s picture

willzyx CreditAttribution: willzyx commented
FileSize
d8-shortcut-save-wrong-message-2469667-1.patch650 bytes
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
CreditAttribution: m4olivei 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
CreditAttribution: Xano commented
Status: Needs review » Reviewed & tested by the community

Nice catch! Thanks :)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott 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
CreditAttribution: m4olivei commented
Status: Needs work » Needs review
FileSize
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 CreditAttribution: 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
CreditAttribution: m4olivei 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 Sydney, Australia
CreditAttribution: jibran 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 🇨🇦
CreditAttribution: webchick at Acquia 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.