Creating content types 'add', 'theme', 'list' (and possibly others) causes various problems

Dunno if it's related : http://drupal.org/node/204119 (filed against D5)

Patch disallows 'add' and 'theme' as content types. I'm not sure what to set as the error message because the string freeze is in effect.

Not duplicate, but related:

http://drupal.org/node/206021

I'm wondering if this might be a menu system bug - needing a more total solution than manually blacklisting reserved words (or if it's not menu system, then there might need to be abstraction of the reserved words somehow).

Bumping status.

subscribe

Ok speaking to chx on irc, this isn't a menu bug. So if it's just bad words then it's not critical either.

The problem is, node_menu() defines menu items under 'admin/content/types/add' (to add content types) as well as under 'admin/content/types/'. $type (to edit content types).

Attached patch changes the URLs so that

• types are added under 'admin/content/types/add'
• types are edited under 'admin/content/types/edit/' . $type
• types are deleted under 'admin/content/types/delete/' . $type.

well - that is one reasonable approach.

Here's another patch I was already working on to just blacklist that small set of types.

http://drupal.org/node/204119 says we should also blacklist "theme". I am not entirely fond of this approach, but it is the least disruptive to Drupal's API/URLs now.

Extended, based on pwolanin's patch.

I was just re-rolling too, but the code comments in this version are better than what I have. Looks RTBC if someone can test.

In my opinion, manually blacklisting a list of words is only a partial solution since other modules may provide menu items that may break the content type administration again. For example, I have installed CCK 6.x from CVS and it provides a menu item admin/content/types/fields, so the word 'fields' should be blacklisted as well.

In the same way, content_copy.module will provide admin/content/types/export and admin/content/types/import, having again two more words to blacklist.

Well, this issue does not warrant changing URL patterns in Drupal all of a sudden either. Drupal uses something/ID/action type of urls, like 'node/2/edit' or 'user/3/track', and the other suggestion here goes against this by moving the action before the ID, so the Id can have the name of the action (in the case of node types, Ids are strings, not numbers).

'theme' has to be blacklisted regardless of the path issue (as '0' already is).

We handled a similar collision with menu module by making paths like:

admin/build/menu/add
admin/build/menu/settings

but for editing custom menus:

admin/build/menu-customize/%menu
admin/build/menu-customize/%menu/edit

http://api.drupal.org/api/function/menu_menu/6

So a solution here could be to change

'admin/content/types/'. $type_url_str

to something like:

'admin/content/node-type/'. $type_url_str

We would then only be changing the paths of callbacks - not any visible menu links.

@pwolanin this is a very good idea, and we will gain consistency around all the administration pages.

If the decision is to continue in the way it is now for Drupal 6.x, the attached patch uses a more restrictive approach. It prevents the use of a content type name that might break the content types administrations page.
In addition to the check performed by the patch provided at #9, it checks if a menu item of the form 'admin/content/types/'. $type->type is already defined, preventing that the new created content type break the administration pages.

Ok, I have set it to "code needs review".
As Gábor said at #8 it is not perfect but it is the least disruptive to Drupal's API/URLs now.

Please, discard the previous patch, use this one instead.

This also verifies that $type->type != $old_type ;)

This patch will open up admin/content/types/ for other modules to append components (now that there is a safeguard). Won't this be a problem? Also, the patch will not have an effect if the module is installed after the content type is created. I suggest going without checking the menu items under admin/content/types/ and filing an issue to CCK.

It does not feel right.

traxer: Yes, you are right, the patch only modifies the content type form validation function to protect existing paths from get broken. If invalids content types has been created before a problematic module installation, then we will have the same problem. CCK is not the only module (package) that will provide local tasks or menu items under admin/content/types, actually any module can do it.

I suggest going with the changes proposed at #13 now and avoid further problems.

I don't like #16 - if it's real a problem, we need to clean up the paths rather than dynamically invalidating paths.

Here's a patch as I suggested above. I'm hoping we don't need a RC1->RC2 cleanup system_update function, since it will at least work fine after a menu rebuild.

It has to be said :
Well Don't Do That Then!

;-)

(I know this is a real issue, and it's cool that some solutions are being looked at, but hey)

#19 looks reasonable. It needs some testing and some look around for other possible code using these paths.

I'd like to stress that there are many reasons why (and thusly many occasions on which) mentioned content types produce problems; hence the change of the title.

+1 Searching contrib HEAD and drupal HEAD for admin/content/node-type produces no hits. We create a virgin menu entry. This should prevent 'list', 'add' and any other type name that conflicted with menu entries from becoming a problem.

A lot of contrib modules use drupal paths like admin/content/types/$type/$action. I don't know whether this might be a problem (considering things like MENU_LOCAL_TASK). Neither did I look at where these modules are on the road to D6 compatibility. I got about 120 hits when searching contrib for admin/content/types. I ruled out 30 as irrelevant (menitioning it without any dynamic parts). I will investigate further.

BTW: Have a look at line 188 of locale.install: drupal_set_message('The language negotiation setting was possibly overwritten by a content type of the same name. Check [...] admin/content/types/negotiation' /*...*/); Made me smile, though I don't know why.

Content types can be created, edited and deleted.

Local tasks of the form admin/content/types/$type/$action from external modules are displayed on admin/content/types. These items work as intended, but e.g. CCK creates a "Manage Fields" local task for each content type.

Won't we get the same problem with admin/content/node-type/ that we now have with admin/content/types/?

A lot of contrib modules have to be modified to accommodate for the change of paths. Therefore, a better path should be chosen. I favour editing content types under admin/content/types/edit/$type, local tasks being admin/content/types/edit/$type/$action. I have attached a patch to implement that path. The patch is simmilar to the one I submitted in #6 except 'theme' is blacklisted and content types are deleted under admin/content/types/edit/$type/delete.

I also attached a patch for CCK to make use of the new paths (in case you want to test my proposal).

Also, there should be some mentioning in the menu documentation, not to mix dynamic paths with fixed paths.

@traxer - I think we should follow the normal Drupal standard, as requested by Gabor.

Also, in terms of:

Won't we get the same problem with admin/content/node-type/ that we now have with admin/content/types/?

The answer is no - contrib modules can safely declare local tasks like admin/content/node-type/$type/$action since there can never be a conflict between $type and $action. Similarly, we don't have to worry about black-listing other type names that might conflict with actions.

Setting to CNW - I favor #19, but it should potentially be re-rolled to provide a default local task to forestall conflict among contrib modules that want to add tabs.

1. #23 follows the standard more closely than #6.
2. #19 shows "Home › Administer › Content management" as breadcrumbs, omitting the "Content type" part on the editing form.
3. By moving the URL to admin/content/node-type/, this problem is solved, but there is no guarantee that similar problems won't arise elsewhere. By adjusting the standard, that risk is reduced. We should not follow self-imposed standards blindly.
4. If admin/content/types/edit/$type is not suitable, how about admin/content/types/list/$type?

@traxer - yes the breadcrumb isn't totally ideal, but it's not too bad. We could fix it up in the page callback.

If Gabor wants to go with your approach, that's fine with me too. However, I don't understand why you're thinking that admin/content/node-type/$type will be more vulnerable to later problems than admin/content/types/edit/$type

The page callback is drupal_get_form(), so a custom callback function will have to be implemented to change the breadcrumbs. I don't think we should change the breadcrumb in the form function ("node_type_form").

However, I don't understand why you're thinking that admin/content/node-type/$type will be more vulnerable to later problems than admin/content/types/edit/$type

Actually it isn't more vulnerable; ... iff devlopers adhere to certain conventions, like not putting dynamic menu items besides static menu items. I find this a reasonable convention to minimize problems.

Certainly, exceptions can be made with good reason, like (as mentioned earlier) with node/add and node/$nid. Those cannot collide, because the $nid is numeric; it is also convenient for the reader of the page.

If this convention is agreed upon, there is no need to open a new menu hierarchy under admin/content/node-type to hold a part of the functionality provided earlier under admin/content/types.

Yes- we'd need a separate page callback to fix up the active trail, but that's trivial if it's desired.

like #19 but with added default local task

It would be great to get some CCK developers here at least. In fact, we are screwing with "their" beloved area, and any change here would make CCK version "X" compatible with Drupal 6 RC2 but not later. (As well as other contribs fiddling with these items).

If/when changes are made in this area, they'll have to be mirrored in CCK menu paths, obviously, and CCK HEAD will then be unusable on RC2. I'm not sure that's a real problem. I mean, there's not even a 6.x branch for CCK yet, so assuming current CCK testers use latest core HEAD *and* CCK HEAD is fine by me.
We hope to open a branch pretty soon (won''t even be an alpha release, because the upgrade path will most probably still be shaky) and 'officially' invite testers. At that moment, it would indeed be a good thing if the menu changes are part of the latest core RC to-date.

Aside from that, I'm not sure I clearly balance the options at hand here.
What I do know is that CCK paths can get deeply nested ('admin/content/types/'. $type_url_str .'/fields/'. $field_name .'/remove' - OK, that page is a simple MENU_CALLBACK, so it could use a simpler path if needed). Any solution adding a depth level at the beginning of the path move us closer to the max_depth...

@Gabor - per yched, I agree that a solution that requires adding an extra part to many paths in not a great idea. Hecne, my patch which doesn't.

Do we need a breadcrumb fixup?

I did not find anything about max_depth. Where should I look?

Considering that breadcrumbs in the menus module don't work as I expected (see admin/build/menu-customize/navigation/edit), I don't think fixing the breadcrumbs will be necessary.

In this case, MENU_MAX_PARTS is the issue (depth applies to links not router items)

http://api.drupal.org/api/constant/MENU_MAX_PARTS/6

I guess this pretty much rules out my suggestion. What is the rationale behind such a constant?

@traxer - the new menu system makes some tradeoffs for scalability and speed. See:

http://api.drupal.org/api/function/menu_get_item/6

On every page load we need to search the {menu_router} table, and the number of possible paths we need to look for grows by powers of 2 with each additional part in the path.

patch in #29 still applies cleanly

Tested #29, added new content type with the name 'add', created this type of content, changed workflow settings. Everything work ok for me.

ok, minor re-roll based on suggestions by chx.

Also, found an error in the form description text. for some reason it says "This name must begin with a capital letter..." while there is no validation for this and the D5 version says "recommended" not "must". So, re-worked it to be like D5 with "It is recommended that this name begin with a capital letter...", since adding validation would be an API change.

And, since the change is going to break the error string anyhow, re-rolled it to be more like the other error text.

Tested with PHP 5.2.4

OK I tested this. Validation works, and I checked the previously broken names (add, list) which were all fine. edited and added content types, all appears to be fine, so marking RTBC.

We discussed the form description text in irc, and personally it looks to me like a wording error rather than a code one, so I agree with the change here.

I accidentally changed the meaning of that string in the patch that related to this commit. Thanks for catching that, pwolanin.

looks like a cross-post, so back to RTBC. Thanks for the confirmation Keith!

Oh. Yes, catch and I cross posted; I didn't mean to revert the status. Also, in 41 I should have said I 'mistakenly' changed the meaning, rather than by 'accident'. :)

Committed, needs to be fixed in CCK. Please open a new issue for this in the CCK queue. BTW I modified the invalid type check to use:

if (in_array($type->type, array('0', 'theme'))) {

instead of

$invalid = array('0' => 1, 'theme' => 1);
if (isset($invalid[$type->type])) {

since it looks cleaner and is more towards what we do elsewhere.

@Gabor - I'm not sure that change works - in_array() can do some very odd things and is going to be slower.

well, seems to work, but I still like the first version better.

Needs to be backported to 5.x, probably with 'add' and maybe 'list' as well.

Hmm, backporting to D5 will lead to trickier compatibility issues with CCK releases.
It's fine in D6, since neither drupal nor CCK have any offcial releases, but I think we should carefully consider a D5 backport...

I'll update CCK HEAD shortly.

Also : Since the patch left the content type overview (Administer >> Content Management >> Content Types) on the old 'admin/content/types' path, the new 'admin/content/node-type/story' pages (and the CCK pages below) are left with a breadcrumb of "Administer >> Content Management", which is definitely a regression.

When editing successive content types, which can happen frequently when tuning your types settings for, say, comments or fivestar behavior, you cannot use the breadcrumb to quickly return to the content types list.

(er, I know, I could have reported this earlier had I actually tested the patch before it gets committed...)

Let's fix that before possibly backporting ?

PS : CCK HEAD has been updated.

simple patch for 5.x to blacklist additional problem names.

Agreed with #48.

ok - here is a patch for yched. Fixes the BC and adds a helper function that CCK can use to do the same.

Also, fixes of some of the doxygen.

thanks pwolanin. Works perfectly well.
A few suggestions about _node_type_fix_trail(), though :
- It currently resides in content_types.inc. Could it be moved to node.module to avoid the extra module_load_include() ?
- It takes the $type object, but only uses $type->type. Could it simply accept the $type->type string instead ?

naive question : why isn't menu_get_item('admin/content/types') good enough ?

@yched - this is 6.x - the router items (what come back from menu_get_item()) are decoupled from the menu links (e.g. what comes back from menu_link_load()).

By default, the system save a menu link corresponding to each router item, which makes it a bit confusing.

@yched - I don't think it's worth slowing node mode in order to avoid the include_once() in CCK ofr rare admin screens.

discussion with chx and yched in IRC makes me think that the last patch needs some more work.

#49 for D5 is still essentially RTBC.

I think this code is a little simpler

empty file...

hmm, wonder how that happened?

here's a working patch.

(For non-credit, new patch attached. Corrected two small typos related to "inserting" and "inserted".)

hmm, needs to check - for some reason deleting content types seems broken. maybe and issue in node_menu with callback inheritance since this patch changes one of the page callbacks

fixed delete page problem, and further simplified code.

Works as expected, content type deletion works fine. Sorry, I've been a little behind this week.
Setting to RTBC - thanks pwolanin

@Gabor - if you don't like the BC fixup code, let's just let yched know so he can move it to CCK and I can think about rolling the doxygen fixes in another issue.

pwolanin: yes, in fact I don't think it is a good idea to disrupt the code this much now.

patch in #49 should still be suitable for 5.x

Fair enough, I'll add pwolanin's code to CCK pages. Thx for this.

Issue still exists in drupal 5.7, (with cck module installed).

yched - February 6, 2008 - 05:12
Fair enough, I'll add pwolanin's code to CCK pages. Thx for this.

yched did you apply it to cck module? if you did please change the status to fix.

pwolanin - February 5, 2008 - 21:22
patch in #49 should still be suitable for 5.x

I'm not able to fix it, where should i apply #49, seems it's for drupal6 ?

@sinasalek - the patch in #49 is for 5.x perhaps you misunderstand the bug, it has nothing to do with CCK. Without the patch (and not on a site you care about), create a content type with machine-readable name "add" or "theme"

oops! sorry i didn't see content_types.inc inside modules\node first time i checked so i thought it has something to do with CCK.
anyway issue still exists in D5.7, i can reproduce it and i applied the patch and it works as expected. thanks

The error message is not very good, but 6.x uses it and it does at least provide instructions to repair the vague error.

Committed to 5.x.

aye but translation simpletest is broken. and for good reasons. This means that only node administrators can access this page, 'cos it inherits from admin/content -- previously it inherited from admin/content/types.

Thanks, committed. Should also be committed to Drupal 7.

seems to have been fixed in 7.x separately

And the breadcrumbs are still broken in D6 on admin/content/node-type/page since those patches were lost in the various attempts to backport this. ;) I just found this and CVS blame lead me to this issue number, and I read the issue. I just rerolled #62 so it applies to DRUPAL-6 and confirmed that it indeed fixes the breadcrumb as previously discussed in this issue.

I gotta say, I absolutely love the fact that CVS log messages link to issue numbers and we have the entire history right here. This is one of the best parts of Drupal development...

The breadcrumb issue for D7 is covered by #371476: bread crumb incorrect on node-type admin pages.

Breadcrumbs were fixed via #508570: Restore URL consistency for node types and menus, but won't fix for D6.