Change constant value of MENU_NOT_FOUND to 404 and MENU_ACCESS_DENIED to 403 [#1373670]

Problem

The MENU_NOT_FOUND and MENU_ACCESS_DENIED constant values are meaningless and not useful when encountering their values in captured data and debugging.
The MENU_FOUND constant is obsolete and not used anywhere.

Goal

Give the MENU_NOT_FOUND and MENU_ACCESS_DENIED constant values some (minimal) meaning.
Remove the obsolete MENU_FOUND constant.

Details

The values of these constants are pretty much arbitrary (1,2,3).
The constants are currently used in the router and page callback system to signify non-success conditions for a HTTP request.
In many cases it would be handy if those existing application-level status codes would at least try to match the protocol-level HTTP status codes, so at least there would be some meaning that developers are somewhat familiar with.
The closest status code values for these constants are 404 and 403 respectively.

/**
 * Internal menu status code -- Menu item was found.
 */
const MENU_FOUND = 1;

/**
 * Internal menu status code -- Menu item was not found.
 */
const MENU_NOT_FOUND = 2;

/**
 * Internal menu status code -- Menu item access is denied.
 */
const MENU_ACCESS_DENIED = 3;

API changes

None. All code is supposed to use the MENU_NOT_FOUND and MENU_ACCESS_DENIED constants, not their values.

Comment	File	Size	Author
	drupal8.menu-status-code.0.patch	652 bytes	sun

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

xjm

she/her

English

CreditAttribution: xjm commented 17 December 2011 at 15:14

Status:

Needs review

» Reviewed & tested by the community

Awesome.

Comment #2

neclimdul

He/Him

CreditAttribution: neclimdul commented 17 December 2011 at 15:52

Status:

Reviewed & tested by the community

» Needs review

Const munging doesn't really do a lot(xjm said on IRC this helps dpm()ing the menu system... maybe). I've got some questions though.
Patch related, why aren't we mapping MENU_FOUND to 200 then since that'd be the closest status code to the expected behavior even if its not used? Why isn't "MENU_ACCESS_DENIED" "MENU_FORBIDDEN" if we're using the forbidden code?

And the shortest but possibly hardest to answer... Why not just return HTTP access codes? WSCCI might like that anyways since its aiming to make our routing layer a more flexible REST server so services can really use status codes and that means more status codes. Does that mean more constants?

I realize some of those questions are holding some kittens over the flames so I'll sum up with, does this actually help enough to put it before WSCCI starts poking in there?

Comment #3

xjm

she/her

English

CreditAttribution: xjm commented 17 December 2011 at 16:25

I actually thought this was a step in the direction of actually returning status codes.

Comment #4

sun

German

Karlsruhe

CreditAttribution: sun commented 17 December 2011 at 19:18

@neclimdul:

It's pointless to define a constant that is not used anywhere. There is no "expected behavior." Therefore, MENU_FOUND is and can only be removed.
The scope of this issue is to change the values of the constants, not the constant names. If you want to change the constant names, create a new issue, please.
The current system returns these constants, not undetermined HTTP status codes. Returning HTTP status codes is out of scope for this issue.
This issue is not about changing our routing layer or implementing a flexible REST server, nor about services, nor about introducing more constants.
This patch is ready. Whatever sub-initiative of the WSCCI initiative might be related to this, is not. At the point we're delaying and blocking ready patches on hypothetical vaporware that not even remotely exists I'm going to leave Drupal core development.

Comment #5

chx CreditAttribution: chx commented 17 December 2011 at 20:26

Status:

Needs review

» Reviewed & tested by the community

It was said in public on record that we will release Drupal 8 without WSCCI if that's what it takes. It was also said clearly that it's not only initatives where the development happens. The latter I have repeatedly asked back whether this is official and it is. As of yesterday, "Wait for WSCCI" is not a valid argument. Even more, we are told it never was although that was not my understanding either.

And, WSCCI right now is in limbo, the context system is postponed on the really pie-in-the-sky "super panels" development.

Comment #6

neclimdul

He/Him

CreditAttribution: neclimdul commented 17 December 2011 at 21:37

@sun Thanks for addressing my questions. I'm sorry you don't feel like addressing this as part of a larger problem.

@chx WSCCI is not in limbo, the issues its proposed have just been pigeonholed. Furthermore its state was not brought up in this discussion I was merely trying to bring the changes in this patch inline with its goals. Obviously that's not in the scope of the original poster so I'll cede that we'll toss it up the chain.

I in no way tried to get this to "wait for WSCCI" I was trying to get this solving the problem in the same way, addressing the needs of services and, since its the core goal of the initiative, the needs of WSCCI. Since you don't want to have that discussion apparently I guess I'll just leave the core issue queue again and go to the WSCCI sandbox and keep working there since bringing its perspective to any issues only seems to bring the ire of core developers. You don't have to wait, we'll just resolve the conflicts later.

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 17 December 2011 at 22:53

I've clarified the issue summary.

My follow-up to @neclimdul might sound a bit harsh, but that's not directed to or caused by him. A plethora of issues have been artificially blocked on wscci, effectively halting core development in various areas, essentially waiting for something that does not even exist yet.

I certainly agree that it makes sense to have a larger plan or bigger picture for larger architectural changes, and those need in-depth discussions. However, we need to be very careful to not block smaller changes that are straightforward. And, we also have to retain a healthy understanding of "scope."

Comment #8

Dries CreditAttribution: Dries commented 19 December 2011 at 14:57

I'm comfortable with this patch.

It's a tiny clean up so I strongly doubt it has any meaningful impact on WSCCI so I'm not even sure why it is worth discussing that in the context of this issue.