Any time a user creates a thing and saves, display a link to the thing created in the status.

Crell said that this is simply canonical link for the entity.

I would like us to consider also linking to the listing where you can find it back, but thats harder.

Are those images backwards?

Yes

This is the taxonomy terms.

Thanks

@prajaankit I am not sure what the patch in #7 is intended to do.

This is nodes. This is going to break some tests.

Do we have to special case it for each entity?

I suppose we could try to do this in the base Entity class.

@prajaankit In #14, you reposted the patch I posted in #6, with some spacing added, for no apparent reason. I am not sure what you are trying to accomplish. I am beginning to question your intentions.

@cilefen,

Due to some Unawareness of Patches Process we do such thing,
Now Become Aware of the Process
Great Thanks tim.plunkett (https://www.drupal.org/u/tim.plunkett) which given me right information
About work,

A patch should contain your own work, and you should describe what you have done in the comment, and how it differs from the previous patches on the issue.

Thanks

So the patches need updates for tests that involve node creation, I will create a new patch that has that and support for linking "other things" that users create...

Lots of changes in the tests... New patch based upon #6 and #9

Last patch failed because of incorrect syntax for the test strings. Here's a new one, that hopefully fixes that.

Dear testbot.....

We also need that for content type creation..

More:

* contact form
* feed
* custom block
* role
* comment type
* language

Will add the additional creation message changes now that the patch-19 passed

I am so excited about this :) such a tiny, but major step in connecting the dots.

So attached is an updated patch that will add links to more creation info messages.

The following links to the detail pages are created:

Links created with this issue:

• contact form
• node
• term
• forum
• shortcut
• feed

No link created because the created entities have no "view" pages:

• vocabulary
• role
• comment type
• menu
• content type
• view mode
• form mode
• custom block

Here it makes no sense to provide a link, because the entity has no “view” page. We could possibly add a link (back) to the edit page, but that might break the flow. And that might possibly throw the user into a loop…

One special issue discovered:

• Custom block

That's a new issue though:
On the custom block configuration page you can configure block settings and title, but the description cannot be edited.
-> new issue
Also while testing that I noticed that when you find this block on the custom block list page and click the link to edit it, you will be able to edit title and description, but not the configuration.
-> new issue

Patch updated with more fixes for tests.

Added an access test for the edge case that a user has permission to link to a route that he has no permission to visit.

This looks great and I think it's really handy to have these links in the messages.

What really puzzled me is that edge case for the Shortcut form. Why on earth is it possible to add shortcuts you do not have access to? Smart fix for that one anyway ... :)

I really wonder if assertText is sufficient in the tests. Shouldn't we test for the links as well?

Ok added tests for the presence of a link in the message area.

Great! Nice to see the functionality added by this patch covered in the tests.

I tried all the messages that are not touched by this issue as listed in #26. I also do not thinks it's useful to be presented with a link to the form you just came from after creating one of these items (i.e. vocabulary, role, ...), especially if you end up on the respective listing page which clearly answers the "Where did this thingy go?"-question.

So let's see: Desired functionality - check, tests meaningful and green - check, coding standards - check.

I call this RTBC.

What's apparently still missing are the follow up issues mentioned in #26. I'll research if these issues are already filed and create them as needed.

I found and apapted #2407761: Add a way to edit block content on the block layout page. It should be an easy resolution to both things @pixelmord found in #26

Lots of small code style problems. Should be easy to fix :)

1. +++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   @@ -64,7 +64,11 @@ protected function setUp() {
   +    // See if the creation message contains a link to a feed.
   
   +++ b/core/modules/aggregator/src/Tests/UpdateFeedItemTest.php
   @@ -40,7 +40,11 @@ public function testUpdateFeedItem() {
   +    // See if the creation message contains a link to a feed.
   

   "See if" is a bit strange, "Check that", "Verify that", "Confirm that" all make more sense.

2. +++ b/core/modules/filter/src/Tests/FilterAdminTest.php
   @@ -294,7 +294,12 @@ function testFilterAdmin() {
   +    $this->assertText(t('Basic page @title has been created.', array('@title' => $edit['title[0][value]'])), 'Filtered node created.');
   +
   +
   +    // See if the creation message contains a link to a node.
   

   Nit: two newlines instead of one.

3. +++ b/core/modules/shortcut/src/ShortcutForm.php
   @@ -28,12 +28,24 @@ class ShortcutForm extends ContentEntityForm {
   +    /*
   +     * There's an edge case where a user can have permission to
   +     * 'link to any content', but has no right to access the linked page.
   +     * So we check the access before showing the link
   +     */
   

   We don't use this style of comments within a function body.

   Also: 80 cols violation.

4. +++ b/core/modules/shortcut/src/ShortcutForm.php
   @@ -28,12 +28,24 @@ class ShortcutForm extends ContentEntityForm {
   +    if ($url->access() == TRUE) {
   

   The == TRUE can be omitted.

5. +++ b/core/modules/taxonomy/src/Tests/TermTest.php
   @@ -245,7 +245,10 @@ function testNodeTermCreationAndDeletion() {
   +    $this->assertText(t('@type @title has been created.', array('@type' => t('Article'), '@title' => $edit['title[0][value]'])), 'The node was created successfully.');
   +    // See if the creation message contains a link to a node.
   

   Missing preceding newline?

6. +++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
   @@ -47,7 +51,12 @@ function testVocabularyPermissionsTaxonomyTerm() {
   +
   +
   

   Two instead of one again.

Fixed above mentioned issues.

New patch attached

Just found this. This is such a great little but *very* useful tweak!
Looks like it only needs another code review.

Why not backport this?

1. +++ b/core/modules/aggregator/src/Tests/AggregatorTestBase.php
   @@ -68,6 +68,10 @@ public function createFeed($feed_url = NULL, array $edit = array()) {
        $this->drupalPostForm('aggregator/sources/add', $edit, t('Save'));
        $this->assertRaw(t('The feed %name has been added.', array('%name' => $edit['title[0][value]'])), format_string('The feed @name has been added.', array('@name' => $edit['title[0][value]'])));
    
   

   This test will probably fail because I accepted the assertRaw in HEAD in the conflict resolution.

2. +++ b/core/modules/contact/src/Tests/ContactSitewideTest.php
   @@ -131,11 +131,19 @@ function testSiteWideContact() {
   -    $this->assertRaw(t('Contact form %label has been added.', array('%label' => $label)));
   +    $this->assertText(t('Contact form @label has been added.', array('@label' => $label)));
   +
   +    // Verify that the creation message contains a link to a contact form.
   +    $view_link = $this->xpath('//div[@class="messages"]//a[contains(@href, :href)]', array(':href' => 'contact/'));
   +    $this->assert(isset($view_link), 'The message area contains a link to a contact form.');
   

   There are a few cases such as this one where we should not use different replacement tokens from HEAD unless it is necessary. But it may be necessary. We could probably combine these two asserts into one assert that checks for the link inside the message text. That would be cleaner and better.

Updated the last patch #47 with assertions in AggregatorTestBase.php and NodeCreationTest.php that have the same logic as all others in the patch.
In my opinion it is perfectly ok to run two assertions, one for the text (of the link) and one for the presence of a link with the correct target.

Updated patch with changes in the assertion that let the last test fail.

Patch in #53 applies cleanly to latest 8.0.x-dev, and appears to address the comments from previous reviews, with the exception of cilefen's second comment in #48. However, pixelmord commented in #51 that he felt that it was not necessary to rewrite the two assertions in to one.

When I manually tested, I noticed some inconsistent behaviour in the links, depending on the type of content created:

* When adding a new contact form, term, forum, shortcut and feed the message includes a blue link without any underline.

* When adding article and basic page nodes, the message includes a blue link with dashed underline:

I haven't looked in to the code to understand why there's a difference, but from a usability and accessibility perspective I'd expect them all to be underlined to more prominently look like links.

That's strange. I think in this context we do not add an underline. Why is this occurring?

There is already an existing issue on adding underline.

@nicrodgers, @Bojhan

The difference in link styling is because after creating an article you are redirected to a page that renders in Bartik, the other entity creation forms redirect to a rout that is rendered by Seven.
So that works as it is intended.

However I will post an updated patch that applies to 8.2.x-dev

So here's the updated patch no significant changes, just made it applicable for 8.2.x

Sorry for the lack of an interdiff, just could not write one, because the interdiff tool rejected..

tagging for the sprint

Reworked patch after test failure, my bad, just lost some code when trying to solve the conflicts I found when trying to apply the 53 patch.

New code attached

OhOh, very sorry, I did not test that locally, so I had to put in some more work.
Another updated version attached

Thanks,
that's a great bit of improving usability, and it works fine for contact form, terms forums and feeds, but I've got two question with regard to nodes and shortcuts.

Contact form: After creation, the user is returned to the Contact forms page, and the message links to the display of the new contact form.
Term: After creation, the user is again on a form to add another term, and the message links to the term page.
Forum: After creating a new forum or container, the user is returned to the Forums page where the order can be changed, and the message links to the page of the new forum/container.
Feed: After creation, the user is again on a form to add another feed, and the message links to the feed page.

Node: After creation, the user is forwarded to the new node page itself, with the message linking to that page. That's a bit confusing, so I'm not sure that this link is needed.

Shortcut: When a user adds a shortcut to a shortcut set, then the message links to the added page. I'm not quite sure whether that is really needed, because the page itself is not actually created. In any case, the wording should similar to other messages "The shortcut to foo has been added." If that wording is provided by the Shortcut module, then we can also take that up as part of necessary improving the UI texts of the module as a whole.

I just talked to Pixelmord and for the Shortcut module the wording is provided by the menu and therefore outside the scope of the module. For the nodes: the message might be displayed after a redirect. So it's better to have an additional, unneeded link then no links at all.

I am very excited to see this patch RTBC!

1. +++ b/core/modules/aggregator/src/FeedForm.php
   @@ -16,13 +16,16 @@ class FeedForm extends ContentEntityForm {
   +      $view_link = $feed->link($label, 'canonical');
   ...
   +      $view_link = $feed->link($label, 'canonical');
   

   This exact line is repeated in both the if and else conditions. Unless this somehow changes based on the $feed->save() call, can we just move this up above and do $view_link = $feed->link($feed->label(), 'canonical'))?

   This same feedback would apply to other points in the patch.

2. +++ b/core/modules/aggregator/src/Tests/UpdateFeedItemTest.php
   @@ -35,7 +35,11 @@ public function testUpdateFeedItem() {
   +    $view_link = $this->xpath('//div[@class="messages"]//a[contains(@href, :href)]', array(':href' => 'aggregator/sources/'));
   +    $this->assert(isset($view_link), 'The message area contains a link to a feed');
   

   Rather than assert(isset()), can we just assertTrue() for these?

   ...Or, maybe better, use assertLinkByHref()? Since we are already asserting the text above.

   Edit: I realized after writing that that the link will possibly appear elsewhere on the page, and the patch's current assertion is also checking to make sure it is inside the messages. So just the assertTrue() is better than my second suggestion.

@xjm good points, I changed these assertions in the tests and moved the $view_link variable assignment outside of the conditional.
new patch attached, so let's see if the test are a go.

Always keep forgetting to set this right away...

One small duplicate $view_link passed by.

So tests failed for a couple of reasons, basically all of them were caused by our assessment that we could optimize the code here. Unfortunately all the changes we did with the good intentions to clean-up were wrong :(

So I just renamed the #64 patch to #74 because:

* To move the link creation in FeedForm.php outside of the IF clause is wrong because it only works if the entity is saved before that. That is done in the IF test.
* To use assertTrue is failing because $this->xpath returns a type of \SimpleXMLElement[] and that is no truthy value
* Removing the repetition of the $this->xpath function call in the ContactSidewideTest.php is also not correct, because we have to do the DOM search again, bacause in between the two calls to xpath a new contact form was created, so we need to do this again.

So let us do another review and test run and then we might be close to commiting that @XJM?

It seems @xjm's review in #68 was off the mark a bit.

Setting back to RTBC, as per @ifrik last status in #67.

Multiple heads make better solutions:

So I refactored the FeedForm.php code so we save the feed first (like all other classes affected by this patch implement that pattern). This results in only one call to the method generating the link.

Attached patch

I discussed with @pixelmord and #74 and #76 indeed make sense (and my suggestions did not). :)

Looks like better maintainable code without that if condition having functional side-effects.

+++ b/core/modules/aggregator/src/FeedForm.php
@@ -16,13 +16,16 @@ class FeedForm extends ContentEntityForm {
+    $label = $feed->label();
+    $status = $feed->save();

Looks like these two lines need to be flipped to preserve the earlier logic. Oops!

OK new patch that flips those lines just to be safe if for some reason title gets altered during save.

Yay, tests passed. Looks good.

Awesome! I reviewed this one more time and things look great. Very glad to have this improvement. Committed d9bf408 and pushed to 8.2.x. Thanks!

Thanks! It's these kind of UX improvements that make a big difference to the user experience.

Yay! So great to see this in :)