Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
arg() is evil. It is hard coded devilry. It should be replaced with a context request so that it can be properly mocked, making unit testing more feasible.
This is the most important issue in all of Drupal 8. :-)
Comment | File | Size | Author |
---|---|---|---|
#12 | argument_handler.patch | 23.98 KB | aspilicious |
#3 | argHandler.patch | 3.23 KB | aspilicious |
Comments
Comment #1
pounard$context['arg:0'] ? \o/
Comment #2
aspilicious CreditAttribution: aspilicious commentedHmm after the path stuff this seems not that hard.
I'm only concerend about $path, how will it look?
$context->getValue('arg:0:node/1') ??
Can a path contain a : (I think it's called semicolon in english?)
When the http and path handlers are in code I'll fix this one.
And if I'm not mistaken the next chuck of code will be handled by the context system
Comment #3
aspilicious CreditAttribution: aspilicious commentedHmm this is a little trickier than I thought.
You can call arg($index, $path);
So for context this could be arg:$index:$path.
1) $path can be null ==> arg:$index
2) $path and $index can be null ==> arg
But now comes the tricky part
3) $index can be null ==> arg:NULL:$path
This seems like a nasty problem...
I hacked my way through it by passing a negative index and test for it but that sound sooooooooooooo bad :).
Good news
----------
Arg gets called a lot (+/-20 times on simple test I performed), the context system only gets called once for each combination of $path and $index (5 times on my test), I don't know how the caching works but that is my observation.
note: these are not real bechmarks ;)
Comment #4
aspilicious CreditAttribution: aspilicious commentedBtw its build on top of the path handlers
Comment #5
Crell CreditAttribution: Crell commentedPassing in the path to process is wrong in innumerable ways. :-) An arg context handler should pull from path:system and nothing else, ever, for any reason.
I'm not sure actually what the key should be for this handler. Let's figure that out the issue before we code it.
Comment #6
pounardIf Drupal path handling API ends up in OOP, there is not reason not to keep the
arg()
function as a static helper (part of this API).arg($index, $path)
function signature is totally legitimate, and context handler could use it as backend for splitting the path. Remember that context is only an accessor to contextual data, and is not a replacement for a lower level API.Comment #7
Crell CreditAttribution: Crell commentedNo, but hard-dependencies on random functions are something to be avoided. It's not like the logic in arg() is complex, and I've never actually seen someone pass in a custom path to begin with.
The handler here should not depend no arg().
Comment #8
pounardPath is something in the core of Drupal way, and right now we have no clue how to remove it or how it will evolve. It sounds legitimate to have an API to manipulate pathes (atomic API) that can be used for other use case such as, dunno, admistration screens or configuration handling, or internal menu handling.
In addition to the upper statement, I'd say that keeping an arg() function (but renamed otherwise, in a bigger path handling API) seems something that makes sense.
Regarding this perspective, I don't think removing arg() solve any problem; people that would do raw path handling for some reason would then become dependant of context where they should not.
Comment #9
Crell CreditAttribution: Crell commentedI'm not suggesting we eliminate the Path API. I'm saying that "get a fragment of the request path" is intrinsically a context API task. If you care about that, then you care about context. Period. And the Context API should not have any dependencies on random global functions.
If there's a use case for breaking up an arbitrary string in a similar fashion, and there may be, that's independent of the Context API and out of scope. There should be no dependency either way.
Comment #10
pounardContext handlers will have, per definition, a dependency over the API that gives birth to the data they expose. If a context handler expose a path, path handling API is then a dependency, it doesn't make any sense otherwise.
Comment #11
webchickPer catch, and blessed by Larry, moving this and all other WSCCI issues to the Drupal core queue.
Comment #12
aspilicious CreditAttribution: aspilicious commentedBased on the wscci branch I made a new argument handler patch that removes the arg function and removes the path argument.
I don't say an argument handler has to look like this. I was curious if I could make it work without the path handler and without introducing extra functions.
I'm not going to set this to needs review as this is based on a sandbox branch.
Comment #13
Crell CreditAttribution: Crell commentedDeprecated by the new Symfony-based approach.