Deprecate and remove usages of arg()

I don't think this is minor.

Could you add @param's to the doc?

Not sure about this, but shouldn't these arguments be added to the items' paths hook_menu implementations?

I wanted to ask why you removed the second argument from "return drupal_get_form('comment_admin_overview', $type, arg(4));" in comment_admin(), but in between the code was updated. So you need to do a reroll anyway.

 function comment_menu() {
   $items['admin/content/comment'] = array(
     'title' => 'Comments',
     'description' => 'List and edit site comments and the comment approval queue.',
     'page callback' => 'comment_admin',
+    'page arguments' => array('new'),
     'access arguments' => array('administer comments'),
     'type' => MENU_LOCAL_TASK,

You cant access "approval" this way. You should keep $type = 'new' in comment_admin(). The @param doc for comment_admin() is nice though.

 /**
  * Form builder; Add an OpenID identity.
  *
  * @ingroup forms
  * @see openid_user_add_validate()
  */
-function openid_user_add() {
+function openid_user_add($form, &$form_state, $account) {

@param doc

 /**
  * Menu callback: Generate a form to add/edit a user profile field.
  *
+ * @param $form
+ *   An associative array containing the structure of the form.
+ * @param $form_sate
+ *   An associative array containing the current state of the form.
+ * @param $op
+ *   The operation to provide, either "add" or "edit".
+ * @param $arg
+ *   If $op is "add", $arg is a profile field type. If $op is "edit", $arg is a
+ *   profile field fid.
+ *
  * @ingroup forms
  * @see profile_field_form_validate()
  * @see profile_field_form_submit()
  */
-function profile_field_form($form, &$form_state, $arg = NULL) {
-  if (arg(4) == 'edit') {
-    if (is_numeric($arg)) {
-      $fid = $arg;

You only need to provide @param doc for $op and $arg really. That's how its done elsewhere.

Also you are removing code here (if (is_numeric($arg)) {) that isn't put back. I am not sure about the drupal_exit(); either.

Bumping to d8.

Retitling. arg() is one of the global violator functions that we need to never rely on. Perhaps this should become a meta for killing it entirely?

As I write this there are ~50 instances of arg() left. We should kill them off mostly as part of #1971384: [META] Convert page callbacks to controllers. Let's kill the rest here.

i was promised stuff

we probably want to do something in ArgumentDefaultPluginBase
storing arguments to a property? since most plugins need them
i am tagging for VDC team to get opinions

it helps if i actually add the tag:P

and without fatals:)

sorry for that, i was sure i checked all the files :/

+++ b/core/modules/node/lib/Drupal/node/Plugin/views/argument_default/Node.phpundefined
@@ -32,8 +32,9 @@ function get_argument() {
+    $args = explode('/', \Drupal::request()->attributes->get('system_path'));

I think we can inject the request plugin now in plugins? Right?

+++ b/core/modules/node/node.moduleundefined
@@ -1968,8 +1968,9 @@ function node_block_access($block) {
+    $args = explode('/', Drupal::request()->attributes->get('system_path'));

o_O Can't we create a helper class to replace arg() ??? This is hard to write/read...

Isn't there a Path class in core?

I think we can inject the request plugin now in plugins? Right?

Yes, but i was thinking whether we should actually inject the Request and store the args in a property in ArgumentDefaultPluginBase since their use is very common in default_arguments plugin...does it make sense? thats why i tagged this for vdc team on #8

o_O Can't we create a helper class to replace arg() ??? This is hard to write/read...

Isn't there a Path class in core?

I dont think this is suitable for the path class, since we would need to make it request aware (right now it just does crud)..unless you mean having a general function in Path to explode any path into pieces?

i hope i fixed all tests

#17: drupal-remove_arg-788900-17.patch queued for re-testing.

aspilicious: Yes, it should be hard to write code to grab individual path parts. That helps discourage people from doing so, since you shouldn't be doing so. :-)

Yes, but i was thinking whether we should actually inject the Request and store the args in a property in ArgumentDefaultPluginBase since their use is very common in default_arguments plugin...does it make sense? thats why i tagged this for vdc team on #8

it might even make sense to have it stored on the full views object, as it is used in various different places (like the table style plugin etc.) but otherwise I like the idea to use it just for default argument plugins and probably argument validation plugins.

Oh btw. I couldn't really find something to review, so it feels kind of RTBC.

it needs a reroll of course..i should also do this for views, thanks for feedback dawehner!

#17: drupal-remove_arg-788900-17.patch queued for re-testing.

Attempting a reroll.

Rerolled the patch. There have been a couple of places in the docs which still referred to the arg() function,
so this is killed now as well.

Thanks, Daniel! Quick, before this breaks again. :)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.phpundefined
@@ -113,26 +113,21 @@ public function validate(array $form, array &$form_state) {
-      if (arg(0) == 'admin') {
-        $form_state['redirect'] = 'admin/config/services/aggregator';
-      }
-      else {
-        $form_state['redirect'] = 'aggregator/sources/' . $feed->id();
...
+    $form_state['redirect'] = 'aggregator/sources/' . $feed->id();

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Form/FeedDeleteForm.phpundefined
@@ -41,12 +41,7 @@ public function submit(array $form, array &$form_state) {
-    if (arg(0) == 'admin') {
-      $form_state['redirect'] = 'admin/config/services/aggregator';
-    }
-    else {
-      $form_state['redirect'] = 'aggregator/sources';
-    }
+    $form_state['redirect'] = 'aggregator/sources';

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AddFeedTest.phpundefined
@@ -26,7 +26,6 @@ function testAddFeed() {
-    $this->assertEqual($this->getUrl(), url('admin/config/services/aggregator/add/feed', array('absolute' => TRUE)), 'Directed to correct url.');

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/UpdateFeedTest.phpundefined
@@ -37,7 +37,6 @@ function testUpdateFeed() {
-      $this->assertEqual($this->getUrl(), url('admin/config/services/aggregator', array('absolute' => TRUE)));

I didn't read the whole issue, but are we sure we're okay with doing this?

+++ b/core/modules/dblog/dblog.moduleundefined
@@ -87,7 +87,7 @@ function dblog_menu() {
+  if (strpos(Drupal::request()->attributes->get('system_path'), 'admin/reports') === 0) {

+++ b/core/modules/update/update.moduleundefined
@@ -99,8 +99,9 @@ function update_help($path, $arg) {
+  if (strpos($path, 'admin') === 0 && user_access('administer site configuration')) {

+++ b/core/modules/views/lib/Drupal/views/Plugin/views/display/DisplayPluginBase.phpundefined
@@ -2711,7 +2711,7 @@ public function getSpecialBlocks() {
+    if (strpos(\Drupal::request()->attributes->get('system_path'), 'admin/structure/views') === 0) {

When doing these strpos checks, I think we should suffix with /.
For example, strpos($path, 'admin/') will prevent it from matching "administrator/something".

Let's inject the request and let's rerole the other changes.

+++ b/core/modules/dblog/dblog.moduleundefined
@@ -87,7 +87,7 @@ function dblog_menu() {
+  if (strpos(Drupal::request()->attributes->get('system_path'), '/admin/reports') === 0) {

I meant the slashes on the other side, like admin/reports/
But maybe both are needed, /admin/reports/

i believe the system_path attribute does not have a slash prefix..so this will always fail

system_path doesn't have a leading / for BC reasons. At some point we need to decide what to do about that... but yeah, this isn't the place to change it.

While being sleepy at least another critical was filed: #2028145: Raw default argument still calls drupal_get_path_alias()

Let's did what tim had original in mind.

Let's make the change in Raw() as simple as possible and fix the feed.

agreed. opened #2028505: Provide the request object for ArgumentDefaultPluginBase::getArgument() as argument to fix views plugins

.

Needs a reroll...

git ac https://drupal.org/files/drupal-788900-38.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 27576  100 27576    0     0  11051      0  0:00:02  0:00:02 --:--:-- 13438
error: patch failed: core/modules/views/lib/Drupal/views/Plugin/views/argument_default/Raw.php:9
error: core/modules/views/lib/Drupal/views/Plugin/views/argument_default/Raw.php: patch does not apply

I am sorry, but why?

Given that we do have \Drupal::request(), what is the value of replacing all:

arg();

With:

explode('/', Drupal::request()->attributes->get('system_path'))

The answer is: there is no value, you are doing it backwards.

Yeah I don't really get the patch as it is at the moment either.

arg() could be refactored to use Drupal::request(), then it's just a thin wrapper, that refactoring hasn't happened yet. See _drupal_environment_initialize() for the @todo.

There's lots of places that you need to know if you're on an 'admin' page or similar in core, and we don't have another mechanism for knowing that at the moment, so just requiring people to use the longwinded method to get that information isn't improving things - it means it's going to be harder to find those places and convert them to get proper contextual information later.

Sure, one could implement arg() as a dumb wrapper around the request object in order to support the old model.

The point is that the old model that relies on global state is fundamentally broken and we're moving away from it. Relying on global request data is an effectively deprecated concept. Having a super-easy way to build logic on global state encourages people to do so. It's called "affordance" in usability. People will do what's easy, so make what's "right" easy and what's "wrong" hard. (Classic example: 110v and 220v electrical outlets are physically incompatible so that you don't plus a 110v device into a 220v outlet and start a fire.)

We cannot completely prevent someone from getting at global state, but we should by no means be encouraging it. If you want to get the path... get it from an injected request object. If your logic in some random location is dependent on arg() right now... find some other way to do it. What's that other way? Honestly I don't know. It depends on what that case is. We cannot figure out all possible alternates for each situation because we don't know every where that someone is using arg(). We can only design the affordances to say "don't do this anymore; this is the Wrong Way To Do It(tm)".

If your logic in some random location is dependent on arg() right now... find some other way to do it. What's that other way? Honestly I don't know. It depends on what that case is."

The patch makes no attempt to answer this question for core though, so I'd fully expect contrib modules to follow the same incredibly ugly examples. This isn't about accounting for every possible case, the ones in the patch would be enough.

So lets take arg(0) == 'admin'. We have an api for that already apparently, its drupal_match_path() with the second argument being path_get_admin_paths(). Bit crappy but it does exist despite none of these arg() cases having been converted to it when it was introduced. Since hook_admin_paths() determines what's an admin page, not just admin/ all the places in the patch are doing it wrong, both with and without arg().

And there's helper for that as well, path_is_admin().

So would updating path_is_admin() and switching the relevant use cases here over to that be acceptable to move this patch forward? (I don't think path_is_admin() is a good approach either, but it's better than directly accessing arg().)

I picked arg(0) == 'admin' as the easiest example, there might very well be better API functions for some of the other changes as well.

path_is_admin() isn't a great approach, agreed, but it's the API that we currently have. Switching from one non-API to another non-API skipping over the API that was actually added to encapsulate that logic is not improving anything.

The point is that the old model that relies on global state is fundamentally broken and we're moving away from it. Relying on global request data is an effectively deprecated concept. Having a super-easy way to build logic on global state encourages people to do so. It's called "affordance" in usability. People will do what's easy, so make what's "right" easy and what's "wrong" hard.

I'm not quite sure where you get this from. Every framework that I know of has a concept of "request-local" data that is accessible from everywhere (stored as thread-local data or as greenlet-local data depending on the execution model). This is true for at least Ruby on Rails, Bottle, Flask, Pyramid.

There is nothing "wrong" conceptually about that. Passing the request object all the way down the chain is just an immense hassle for very little gain, because there is only ever one active request at a time in a given context.

Larry stated in an other issue: "Path-sensitive code is a code smell". I do totally agree with that. But the current patch is not a step in this direction: it doesn't make sense to the procedural wrappers until we got rid of the path-sensitive code.

#38: drupal-788900-38.patch queued for re-testing.

So is it a won't fix?

I would say postponed?

Yeah it's postponed - it's OK to remove arg(), but replacing it with one-off hacks that do exactly the same thing isn't. #244090: Tie help into menu router is trying to replace path checks with route checks, which is a step in the right direction.

Reroll attached.

Just 10 usages left..and from them aggregator's 2 are checks which are dead..the path will never be admin, because i removed the duplicate edit/delete route

w/e

Made some more cleanups.

i very much like the dblog cleanup, nice! the key has changed to library though

No clue how this was passing

1. +++ b/core/includes/bootstrap.inc
   @@ -1968,57 +1968,6 @@ function _current_path($path = NULL) {
   -function arg($index = NULL, $path = NULL) {
   

   Needs to be deprecated before it's removed.

2. +++ b/core/includes/theme.inc
   @@ -1981,8 +1981,9 @@ function template_preprocess_html(&$variables) {
   +  $path_args = explode('/', \Drupal::request()->attributes->get('_system_path'));
   

   This should use current_path() for now, to avoid relying on the request attribute. See #2239009: Remove public direct usage of the '_system_path' request attribute.

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedForm.php
   @@ -66,12 +66,7 @@ public function save(array $form, array &$form_state) {
          drupal_set_message($this->t('The feed %feed has been updated.', array('%feed' => $feed->label())));
   -      if (arg(0) == 'admin') {
   -        $form_state['redirect_route']['route_name'] = 'aggregator.admin_overview';
   -      }
   -      else {
   -        $form_state['redirect_route'] = $feed->urlInfo('canonical');
   -      }
   +      $form_state['redirect_route'] = $feed->urlInfo('canonical');
        }
        else {
   

   Is this functional change OK? We have the route admin context available now.

thanks for the review!

fixed 1 and 2.

About 3: This is dead code that i forgot to remove in #2152673: Remove duplicate edit route from aggregator and make delete/add consistent with rest of core
where i removed the admin routes that add/edit feeds. That means that arg(0) == 'admin' will always evaluate to false there.

68: drupal-arg-788900-68.patch queued for re-testing.

Straight reroll.

Patch looks great and removes all usages of arg() as advertised. Response in #68 is correct. Just fixing 2 docs issues:
- suggestions are not generated in preprocess functions anymore, so removed the legacy doc
- there was an arg() reference in a code comment for the Views Raw plugin, so tried to improve that doc

Otherwise, if this passes bot, I think it's RTBC.

@@ -25,7 +26,8 @@ class UserLoginBlock extends BlockBase {
+    return ($account->isAnonymous() && !in_array($route_name, array('user.register', 'user.login', 'user.logout')));

Would be good to use a strict comparison here (TRUE).

For a very limited set of comparison values, it would be even better to compare each value separately, so as to avoid the cost of array() + in_array().

In this particular case, we could even optimize further:

if (!$account->isAnonymous()) {
  return FALSE;
}
$route_name = ...;
return ($route_name !== 'user.register' && ...);

sun: That's an obscene level of micro-optimization. The existing code is far more readable than a long line of separate booleans. I don't know what cost of array() + in_array() you're talking about, but if it even exists go eliminate an SQL query and I promise you'll make up for it 100 fold. :-)

thanks for the doc fixes @effulgentsia
this is good to go now, just needs the RTBC flag

I'll take #76 as approval of #73's interdiff, so RTBC. I agree with #75 that #74 is not worth it: login block access is checked once per request: this isn't where those kinds of micro-optimizations are worthwhile.

I'm not sure that the change notice is very helpful - https://drupal.org/node/2274705. I'm sure we'd want to discourage people from doing

  $path_args = explode('/', current_path());

to often - right? I don't think we're deprecating arg() in favour of this approach.

yeah, i started it yesterday and left it as placeholder - its definitely not finished, but i will finish it later today

i added more examples and a final note https://drupal.org/node/2274705

d.o ate my comment... reuploading.

OK there's still ugliness here (current_path() explosion and route location) - but we have a critical issue for getting the current route and it's much, much better than a year ago when it should never have been initially RTBCed...

The current path explosion I'm not convinced we even need those theme/template suggestions so opened #2275487: Remove current path parts from theme suggestions, maybe we can just ditch that.

Committed/pushed to 8.x, thanks!

a