Everyone keeps saying, "I always wondered about that, but figured some smart person decided it for a reason..."

CommentFileSizeAuthor
#19 drupal8.node-type-url.19.patch37.96 KBmatt2000
#17 drupal8.node-type-url.17.patch37.81 KBmatt2000
#15 drupal8.node-type-url.15.patch30.21 KBsun
#13 drupal8.node-type-url.13.patch28.26 KBsun
#11 drupal8.node-type-url.11.patch28.27 KBsun
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented

This was born from: http://drupal.org/node/617532#comment-2203464

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Remove underscore-to-dash conversion in URL references to content types » Remove underscore-to-dash conversion in path arguments for content types
Version: 8.x-dev » 7.x-dev
Category: feature » bug
Priority: Minor » Normal

Ultimately, I'd like to see this fixed as part of #617532: field_ui menu paths doesn't translate underscores in content type names, since this is a fake limitation (and therefore a bug) that never made any sense.

yched’s picture

yched CreditAttribution: yched commented

Wouldn't it break links on existing Views using a 'node:type' argument after a D6 -> D7 upgrade ?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 7.x-dev » 8.x-dev

webchick overruled the timeline, but provided some background info:

this underscore-to-hyphen thing was asked for by Dries back when the node type part of CCK first got into core in D5. I don't fancy breaking everyone's URLs at this point, so agree with moving this discussion to D8.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Version: 8.x-dev » 7.x-dev

However, this totally awkward conversion of internal ids can cause a range of unexpected bugs. Identifiers should be unique and not munged in any way.

So let's fix this.

yched’s picture

yched CreditAttribution: yched commented

Marked #605236: Content type - bundle realname not using machine readable name as duplicate.

I'm not sure I see which clashes you expect. node type names only allow 'a-z_', so converting '_'s to '-'s should not cause any conflict.

sun.core’s picture

sun.core CreditAttribution: sun.core commented
Version: 7.x-dev » 8.x-dev
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Category: bug » feature

Only feature requests and tasks can be assigned to D8 for now, because the Drupal 8 tree is not opened yet.

Fortunately, this is a feature request :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented

Subscribing, why did we introduce this conversion in the first place? Is a '-' in a URL prettier than a '_'?

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Component: node system » node.module
Category: feature » task
Issue tags: +Entity system, +Novice

Any takers? This should be relatively simple (just grep entire core for the string replacement and remove them all), so tagging as Novice.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Active » Needs review
FileSize
drupal8.node-type-url.11.patch28.27 KB

Kick-starting this, 'cos I can't stand this bogus beautification anymore.

Status: Needs review » Needs work

The last submitted patch, drupal8.node-type-url.11.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
Issue tags: +API change, +API clean-up
FileSize
drupal8.node-type-url.13.patch28.26 KB

Created follow-up issue for #1564832: Remove _node_extract_type() and node_type_get_name() (postponed on this one)

Status: Needs review » Needs work

The last submitted patch, drupal8.node-type-url.13.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Needs work » Needs review
FileSize
drupal8.node-type-url.15.patch30.21 KB

Fixed the exceptions.

Status: Needs review » Needs work

The last submitted patch, drupal8.node-type-url.15.patch, failed testing.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented
Status: Needs work » Needs review
FileSize
drupal8.node-type-url.17.patch37.81 KB

Here's a new patch with fixes for the broken tests. (That is, the new code has not changed; instead this patch only includes updates to old tests that try to get a 'test-bundle' url instead of the new 'test-bundle'.)

The last submitted patch, drupal8.node-type-url.17.patch, failed testing.

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented
FileSize
drupal8.node-type-url.19.patch37.96 KB

I guess my patch generation method needs updating... Let's try this one.

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented

woot! Thanks a lot @matt2000!

@chx requested in IRC to dig out why this conversion exists in the first place. Answer: Purely cosmetic.

Introduced in the initial commit for custom node types for D5: #62340-91: Pave the way for CCK

And even back then, a lot of discussion followed on that "beautification" detail already (search the issue for "dash"). :(

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

OK then.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Title: Remove underscore-to-dash conversion in path arguments for content types » Change notification for: Remove underscore-to-dash conversion in path arguments for content types
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active

Oooh I'd not seen this issue. Committed/pushed to 8.x, will need a change notification.

19 files changed, 97 insertions(+), 131 deletions(-)
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review

Opened two change notices:
http://drupal.org/node/1574668
http://drupal.org/node/1574670

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Change notification for: Remove underscore-to-dash conversion in path arguments for content types » Remove underscore-to-dash conversion in path arguments for content types
Priority: Critical » Normal
Status: Needs review » Fixed

Thanks! I've revised both a bit.

Status: Fixed » Closed (fixed)

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

joachim’s picture

joachim CreditAttribution: joachim commented

> Is a '-' in a URL prettier than a '_'?

Yes. Much.

Wish I'd seen this ages ago, as this would have got a big fat -1 from me :(

matt2000’s picture

matt2000 CreditAttribution: matt2000 commented

@joachim

The bigger issue, IMO, is consistent Developer experience. The conversion is an unneeded WTF. So a follow-up patch to change the entity machine name convention to use dashes instead of underscores would be fine with me -- but I'd expect more resistance from others by going in that direction, as it seems to me it would have much more far reaching consequences than standardizing the URLs this way.

joachim’s picture

joachim CreditAttribution: joachim commented

It's a big change to user-facing parts of Drupal that seems to have gone through without that much discussion.

I feel there are much bigger, further-reaching WTFs in Drupal. I for one have never been tripped up by this one. You're either dealing with a path argument or making a URL --> use a hyphen, or you're dealing with $info and the database --> use an underscore. The only places you need to go from one system to the other are menu loaders, which I would hope in D8 are unified to just one grand entity_load(), and making links, where we could have added a helper function.