When you delete a site through the UI, the node alias that we seem to have now, somehow is removed, resulting in a 404. The site node itself is not deleted, which is fine.

CommentFileSizeAuthor
#5 node-alias-908314-5.patch815 bytesjoestewart
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

This is generated on hosting_context_register, and we remove the hosting_context for items that are deleted, and so we should.

So I guess our ajaxy refresh stuff needs modification, to refresh and take us back to the node itself and not the alias.

... or can we just get rid of the aliases altogether? :)

Alternatively we can just not delete the url_alias on hosting_context_delete(): granted that leaves some cruft in the database, but I'd prefer cruft than 404s.

/**
 * Delete an alias from the context lookup table.
 */
function hosting_context_delete($nid) {
  db_query("DELETE FROM {url_alias} WHERE src='node/%d'", $nid);
  db_query("DELETE FROM {hosting_context} WHERE nid=%d", $nid);
}
helmo’s picture

helmo CreditAttribution: helmo commented

Just for completeness this also happens when a platform is deleted. And probably also when a server is deleted.

It would be nice to get a confirmation on this page something like: "The [site|platform|server] has been deleted."

Either that or redirect the user to a listing page, e.g. the list of sites, platforms or servers.
But such a redirect could also cause more confusion.

helmo’s picture

helmo CreditAttribution: helmo commented

Marked #922538: Deleting Sites / Platforms results in 404 as duplicate of this issue.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Priority: Normal » Major
Issue tags: +aegirWTF
joestewart’s picture

joestewart CreditAttribution: joestewart commented
Status: Active » Needs review
FileSize
node-alias-908314-5.patch815 bytes

How about just leaving the alias alone since the node isn't deleted?

git format-patch style patch for 7.x-2.x attached for review.

helmo’s picture

helmo CreditAttribution: helmo commented

@joestewart: Wouldn't that land you on the site node page of the site you just deleted?

There would have to be a very clear visual indication that the site no longer exists.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

That's what used to happen anyway. The only indication was the 'Status' in the info within the node body. Maybe we need a drupal_set_message() or something on sites that are considered deleted, or a css change of some description.

helmo’s picture

helmo CreditAttribution: helmo commented
Status: Needs review » Needs work

A drupal_set_message() would be a nice way to report the results of a delete action after a redirect to a listing page.

For the old site node, that has status "deleted", a drupal_set_message() would be less appropriate.
Marking the text "deleted" bold and/or red might be a way to make that clear.

joestewart’s picture

joestewart CreditAttribution: joestewart commented
Status: Needs work » Needs review

Thanks. I'm going to put this back to needs review since the patch fixes the regression. This returns to the previous behavior without error.

Maybe some change in display/notification is needed other than the "Status: Deleted" that is already shown. But it seems beyond a simple fix. As skwashd noted in #922538: Deleting Sites / Platforms results in 404 the javascript in hostingTaskListRefreshCallback is where the reload happens.

anarcat’s picture

anarcat CreditAttribution: anarcat commented

I agree. Can anybody confirm this is not breaking anything else? I'm especially concerned about removing the context but *not* the alias...

joestewart’s picture

joestewart CreditAttribution: joestewart commented

Just to be clear, we are referring to path alias and not drush alias. The node still exists, so why not be able to refer to it by it's url alias.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

I'll test this this week, I'd love to see it get into 1.2.

From memory there was some concern about leaving the alias in the db, something to do with if you add a new site with the same URL and whether there's some sort of clash there when the new context with the same name is created?

omega8cc’s picture

omega8cc CreditAttribution: omega8cc commented
Status: Needs review » Needs work

The alias must be deleted to avoid collision when you will create the site with the same domain name again, so we can't use the patch from #5.

omega8cc’s picture

omega8cc CreditAttribution: omega8cc commented

Or maybe we could force deletion of the old alias (if exists) when the site or platform is created? Then we could avoid deleting the alias when the site/platform is deleted.

joestewart’s picture

joestewart CreditAttribution: joestewart commented

Sorry I can't duplicate any problems whether recreating a site on the same platform, different platform, nor creating another platform with the same name as the deleted platform.

Thanks for the review.

omega8cc’s picture

omega8cc CreditAttribution: omega8cc commented
Status: Needs work » Needs review

My bad, I didn't try the patch, so maybe it works already as I expected. I will try it but for now I'm reverting the status.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Fixed

I tested this and was satisfied with the result. There is no unexpected behaviour (creating a new site with the same name) and the experience is a lot better than a 404.

Granted that when you create a new site, you lose the history of the previous site's node & tasks because it 'takes over', but that probably happened / happens already despite this bug. (Edit: it's not lost, it's just the alias takes over for the new node, and if you know the old site's nid, you can still see the history of ran tasks no problem)

I've committed this to the 6.x and 7.x branches.

Thanks Joe Stewart!

Status: Fixed » Closed (fixed)

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

Steven Jones’s picture

Steven Jones CreditAttribution: Steven Jones commented

So I've worked on the proper fix for this one over here: #1256508: path aliases for deleted sites are not deleted should mean that you can easily access old sites as well as new ones!

  • Commit afe584f on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x authored by joestewart, committed by mig5: 
    Issue #908314 - Stop deleting node alias

  • Commit afe584f on 6.x-2.x, 7.x-3.x, dev-ssl-ip-allocation-refactor, dev-sni, dev-helmo-3.x authored by joestewart, committed by mig5: 
    Issue #908314 - Stop deleting node alias