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.
A lot of low level components need ip_address(), but it's a procedural function in bootstrap.inc so they currently have a direct dependency on it. This could be factored out into the DIC somewhere.
Comment | File | Size | Author |
---|---|---|---|
#97 | drupal-remove_ip_address-1847768-97.patch | 33.58 KB | ParisLiakos |
#93 | drupal-remove_ip_address-1847768-93.patch | 33.57 KB | ParisLiakos |
#93 | interdiff.txt | 489 bytes | ParisLiakos |
#91 | drupal-remove_ip_address-1847768-91.patch | 33.57 KB | ParisLiakos |
#90 | drupal-remove_ip_address-1847768-90.patch | 33.94 KB | ParisLiakos |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedSymfony already have some primitive support for this in the Request object, we should just remove our implementation.
Comment #2
catchDo they have any reverse proxy support then?
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes, they have support for
Client-Ip
(that we don't have) and some primitive support forX-Forwarded-For
. You need to callRequest::trustProxyData()
for that to happen.Comment #4
sunThe main issue I see is that we do not have any tests for
ip_address()
, but we do have generated plenty of expectations over the years.Thus, there's a good chance for regressions here — not necessarily in the main/trivial functionality, but rather in the advanced reverse proxy handling.
Comment #5
Crell CreditAttribution: Crell commentedIn the absence of existing tests or a clear list of current functionality, we should switch to the Symfony version and let the chips fall. Add tests on top of the new one, and if someone flags a regression we can address it then, possibly pushing upstream. We cannot let ourselves be held back by "we don't know what the capabilities are, so we can't replace anything".
Comment #6
catch#142773: Drupal does not fully work when using a reverse proxy, #258397: IP address identification not broad enough and #621748: ip_address() is broken for multi-tier architectures describe most of the functionality in the current ip_address(), there's only five commits total according to git blame.
So it feels like it shouldn't be particularly difficult to write tests for the main points retrospectively then apply those to the Symfony code and see if it handles it. Two of those issues were marked as critical bugs so I'd prefer not to hit and hope in terms of regressions.
Comment #7
Damien Tournoud CreditAttribution: Damien Tournoud commentedWe have full tests for this in
Drupal\system\Tests\Bootstrap\IpAddressTest
, but we should just remove them. The actual features are completely irrelevant. We have chosen to use Symfony for our core framework, we are not going to fork it. If you want a particular feature to be implemented, open a PR request against Symfony.Comment #8
catchUpstream PRs are fine but they need to be done before actually using the thing being PRed.
Comment #9
catchTurns out when you review things before you use them (thanks Damien for actually doing so), you find bugs:
http://symfony.com/blog/security-release-symfony-2-0-19-and-2-1-4
At the least $conf['reverse_proxy_address'] can be supported now.
Comment #10
Crell CreditAttribution: Crell commentedOur Symfony code has been re-upped, so let's get back to this issue: #1879410: Update Symfony dependency
Comment #11
cgreaten CreditAttribution: cgreaten commentedComment #12
ParisLiakos CreditAttribution: ParisLiakos commented@cgreaten are you working on this?
Comment #13
cgreaten CreditAttribution: cgreaten commentedComment #14
ParisLiakos CreditAttribution: ParisLiakos commentedlets see..
Comment #16
ParisLiakos CreditAttribution: ParisLiakos commentedComment #18
ParisLiakos CreditAttribution: ParisLiakos commentedInstalling fails cause
_drupal_session_write
tries to write in database an entry with NULL hostname..apparently somehow Symfony'sRequest
is initialized without any parameter..So naturally getClientIp() returns NULL..but hostname field cant be NULL..If i hardcode the hostname to
''
, install goes on successfully.i got lost trying to figure out whats wrong..any help appreciated
Comment #19
ParisLiakos CreditAttribution: ParisLiakos commentedThis in
_drupal_session_write()
gets me past the installer at leastAlso, i am almost sure that my approach with flood classes is wrong, will figure it out another day though
Comment #21
Damien Tournoud CreditAttribution: Damien Tournoud commentedShooting in the dark here. But possibly we currently write to the session *after* the container has teared down?
Comment #22
ParisLiakos CreditAttribution: ParisLiakos commentedpicking this up again
Comment #23
ParisLiakos CreditAttribution: ParisLiakos commentedThis is probably THE hack, but thats the only way i could find to make installer work
Also what should i do with flood classes?
Probably this is not ideal:
So, should i make identifier a required parameter, or inject the request object in the class, with getters/setters, i am pretty sure, i can't inject it from the Bundle since request is synthetic
Comment #24
ParisLiakos CreditAttribution: ParisLiakos commentedMade the hack prettier to read
Comment #25
ParisLiakos CreditAttribution: ParisLiakos commentedah, crap i am sorry, previous patch will throw a fatal error, so my dear admin can you also cancel the test above and let this run?
thanks and *really* sorry for the noise
Comment #27
ParisLiakos CreditAttribution: ParisLiakos commented#25: drupal-remove_ip_address-1847768-25.patch queued for re-testing.
Comment #29
ParisLiakos CreditAttribution: ParisLiakos commentedlets see if this fixes simpletest
Comment #31
ParisLiakos CreditAttribution: ParisLiakos commentedUrm, patch above misses reverse proxy subscriber:/
Comment #33
ParisLiakos CreditAttribution: ParisLiakos commentedOk lets see this one..still need some advice with flood classes, i am not sure whats the best way to act there
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedThis should go green..i had to update the request in module_enable|disable cause the kernel was being rebuilt
Comment #36
Crell CreditAttribution: Crell commentedThe flood backends should be injectable now, so rather than making a static call to Drupal::service to get the request just add it as a dependency in the constructor.
Same here.
Other than that, this looks great!
Comment #37
Crell CreditAttribution: Crell commentedComment #38
ParisLiakos CreditAttribution: ParisLiakos commentedHmm MemoryBackend is not a service..but, i just read the docblock and it is saying it is only for testing..so i pass the request when the test instantiates it..
DatabaseBackend, i did
I thought it wouldnt work, but well i dont seem to get any error and flood tests pass..so i guess it is ok
Comment #39
Crell CreditAttribution: Crell commentedYay!
Comment #40
Damien Tournoud CreditAttribution: Damien Tournoud commented^ This break encapsulation. We should create a dummy request instead.
^ Those two need to move somewhere else, presumably in
DrupalKernel::initializeContainer()
, and they need to be done in a way that doesn't break encapsulation: we should just save the previous request if we are in a request scope already.Comment #41
ParisLiakos CreditAttribution: ParisLiakos commentedComment #42
ParisLiakos CreditAttribution: ParisLiakos commentedIndeed! i think its way better now..thanks @katbailey for some hints and damien for explanation..lets see if bot agrees too
Comment #43
Damien Tournoud CreditAttribution: Damien Tournoud commentedThanks! This starts to look great.
^ Don't we also need to start a request scope?
^ Do we need both? Isn't the second one already covered by the code from
DrupalKernel::initializeContainer()
?Comment #44
Damien Tournoud CreditAttribution: Damien Tournoud commentedAlso, I wonder if it actually makes sense to start a request unconditionally like that in the test harness. Arguably, if a piece of code rely on a request being set without having a good reason for it, it's a bug in that particular piece of code... Tests should not depend on a request being set on the testing side, and that's true for both unit tests and web tests.
Comment #45
ParisLiakos CreditAttribution: ParisLiakos commentedwow, i just removed both additions in simpletest and run a few tests, seems everything is working oO..seems all i had to do instead of hacking around was just to fix the kernel:/
thanks again damien..
lets see what bot says
Comment #47
ParisLiakos CreditAttribution: ParisLiakos commentedok, this failed only in upgrade path tests so i only set a request there, and flood test where i dont touch the container, just inject a dummy request..
i think this time i got it right:)
Comment #49
ParisLiakos CreditAttribution: ParisLiakos commentedthat view test should be a random failure, i cannot reproduce it locally..i had to make an adjustment for the flood tests
Comment #50
Damien Tournoud CreditAttribution: Damien Tournoud commentedLooks really good to me now.
Comment #51
podarokthis one needs change notification
+1RTBC
Comment #52
xjm#49: drupal-remove_ip_address-1847768-48.patch queued for re-testing.
Comment #53
webchickSince we're adding our own code for ReverseProxySubscriber, it seems like we at least need test coverage for that.
I took a spin through core/vendor/symfony/http-foundation/Symfony/Component/HttpFoundation/Tests/RequestTest.php and there seems to be fairly robust test coverage of most of the things testIpAddress() used to be doing, so hopefully we're covered there.
This doesn't need a change notification yet, until it's committed.
Comment #54
ParisLiakos CreditAttribution: ParisLiakos commentedthanks to msonnabaum for the initial hints:)
also found out that the request methods are static..thanks phpunit:P
Comment #55
ParisLiakos CreditAttribution: ParisLiakos commentedoops, that trailing .php shouldnt be there:P removed
Comment #56
Crell CreditAttribution: Crell commentedShortdesc of the docblock is not one line. This is why we only use the short name of a class in description text. :-) (And we can probably be even shorter than that here, and go into more detail in a longdesc if we really really need.)
Same.
Comment #57
ParisLiakos CreditAttribution: ParisLiakos commentedComment #58
Crell CreditAttribution: Crell commentedHuzzah, PHPUnit tests!
Comment #59
xjmFiled #1957156: Add a request() method to \Drupal.
Comment #60
webchickI literally cannot bring myself to commit a patch that does something this horrific to DX, so I will leave it for catch or alex. :P
Comment #61
gregglesWhy not
Comment #62
tim.plunkettThat, plus a nice
@deprecated
tag, and call it even? That's what we've done elsewhere.Comment #63
Dave ReidCan we at least deprecate the functions like this for now rather than just plain remove?
Comment #64
ParisLiakos CreditAttribution: ParisLiakos commentedi thought we were against wrapper functions..and want to state that this patch blocks other things
#1960344: Replace $is_https global with Request::isSecure() and eventually #1858196: [meta] Leverage Symfony Session components
Comment #65
tim.plunkettFor your consideration.
Comment #66
ParisLiakos CreditAttribution: ParisLiakos commentedComment #67
Crell CreditAttribution: Crell commentedIt's late so I won't write a long screed, but I have to take exception to webchick's comment in #60. We're talking about a rarely accessed value that the vast majority of code not only doesn't need but *shouldn't* need. Which in most cases can and should be accessed more like this:
What the "longer" version going through Drupal:: for legacy code gets us is the ability to do actual unit testing. That was the main argument for moving from drupal_container() to Drupal::service(): The code lazy loads which means we can actually talk about proper testing. Anyone using ip_address(), even (especially) as a dumb wrapper around Drupal::, is actively saying "I want to actively work against testability of my code." We really need to stop doing that.
#57 is the patch to commit.
Comment #68
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes, we *really* need to stop fighting. We cannot ship Drupal 8 has an horrible mismatch of procedural and dependency-injected code. If we do that, everyone loses: the seasoned Drupal developers that are going to have to learn completely different paradigms, and the seasoned Symfony developers that are not going to want to touch our horrible beast with a ten-foot pole.
#57 is definitely the patch to commit.
Comment #69
Damien Tournoud CreditAttribution: Damien Tournoud commented(for the same reasons, we really cannot ship with *both* PHPUnit and Simpletest, *both* Symfony container and Drupal bootstrap, *both* hooks and events, *both* routes and hook_menu)
Comment #70
Dave ReidOk then as a developer, I would not want this to land until I know for 100% certainty that #1957156: Add a request() method to \Drupal will land.
Comment #71
msonnabaum CreditAttribution: msonnabaum commentedAgree with #70. It's not about procedural vs DI, it's about not introducing unnecessary complexity at the API level.
Comment #72
ParisLiakos CreditAttribution: ParisLiakos commentedLets get #65 in and then remove ip_address() in #1957156: Add a request() method to \Drupal and be done with this issue?
Comment #73
Damien Tournoud CreditAttribution: Damien Tournoud commented@Dave, msonnabaum: as developers, you are going to use:
In whatever service you are using it from (ie. the
$this
).Drupal 8 cannot ship if you still have major needs to access the request from the
Drupal::
global.Comment #74
Dave ReidSo you can personally guarantee that on behalf of this tiny fraction of contrib modules that already call ip_address() that they would only be calling it in service or controller context and not from any kind of hook?
Comment #75
effulgentsia CreditAttribution: effulgentsia commentedWhy not? Per #1957156-11: Add a request() method to \Drupal, Drupal::request() would give you the right $request object, even in subrequests. The only downside is unit testing, but even for that, the unit test can do a faux subrequest in order to get a desired $request object into the global container.
Comment #76
webchickI think we should probably postpone this on #1957156: Add a request() method to \Drupal and sort out what we want to do here.
Comment #77
Crell CreditAttribution: Crell commentedDave: As noted in the docblock for the \Drupal class, you can factor your hook out to a service, too. (That service therefore being injectable.)
But any place a hook is accessing the request object, the first question should be "Why? Do I actually need to? Is there some way I can do this that doesn't tightly couple me to an HTTP request and therefore prevent my code from being used in Drush without shenanigans?"
Comment #78
effulgentsia CreditAttribution: effulgentsia commentedBased on #1957156-14: Add a request() method to \Drupal, I think we need more discussion on $request in general, when it is/isn't appropriate to use, and how to fix the code that uses it inappropriately. I think some of the uses of ip_address() (e.g., in ban_ip_form_validate()) present excellent concrete examples for us to deal with in figuring that out.
Meanwhile, some of the tests (e.g., CommentLinksTest) seem to call ip_address() for no good reason. We don't need those tests passing the ip address of whatever launched simpletest; we can replace those with mock values, like '127.0.0.1'.
So how about this? We move forward with:
- the part of #57 that replaces our custom implementation of ip_address() with using Symfony's.
- the part of #57 that uses $request where there's a correctly injected one.
- replace CommentLinksTest and similar to use '127.0.0.1'.
- leave ip_address() as a wrapper for the places that require more discussion, and open follow ups to figure out how they can do it the right way.
That's what this patch does. In other words, it side steps the question of Drupal::service('request') vs. Drupal::request(), since the entire premise of hooks accessing $request at all is still being debated in #1957156: Add a request() method to \Drupal.
Comment #79
Crell CreditAttribution: Crell commented+1 on changing the tests to not care either way; they shouldn't, that's a good move.
-1 on continuing to use ip_address(). Drupal::service('request') is going to continue to work no matter what happens in #1957156: Add a request() method to \Drupal, and the only discussion still in that thread is about a comment, not the code.
If we must keep ip_address() in existence for a little longer, at the very least that's what @deprecated is for. Its death is a release blocker.
Comment #80
effulgentsia CreditAttribution: effulgentsia commentedHow can we deprecate it if we don't know what we're replacing it with? With #78 applied, ip_address() is called from the following places:
- ban_ip_form_validate()
- CommentStorageController::preSave()
- openid_verify_assertion_nonce()
- user_login_authenticate_validate()
- watchdog()
- drupal_anonymous_user()
- _drupal_session_write()
I'm fairly confident the last one will get fixed by moving to Symfony's session handling. I'm somewhat confident that the two above that can be fixed as well, somehow. But I'm not at all clear on how we fix the first 4. And if we don't yet have a solution for those, how can we call it deprecated?
Yes, but isn't the claim in #1957156-14: Add a request() method to \Drupal that setting the precedent of calling that is even worse than calling ip_address()? Core needs to follow its own rules: I don't see how we can say that calling Drupal::service('request') is wrong, but then do it in a bunch of places with no clue on how to do it the right way. Or am I the only one who's clueless on how to do those first four the right way?
Comment #81
Crell CreditAttribution: Crell commenteds/ip_address()/Drupal::service('request')->getIpAddress()/
s/ip_address()/Drupal::request()->getIpAddress()/
Both of those are equally better than ip_address(), because they're lazy-loadable and, kinda sorta, testable (in some tests, anyway). I think Damien is over-reaching when he says that ESI is impossible as long as Drupal:: exists; it's harder, and maybe buggier if you're not careful, but not impossible.
Which of those two changes we make is purely a DX/convenience switch. And the only thing holding up the linked issue (which I marked back to RTBC) is how firmly to discourage (but not prevent) people from calling Drupal::request().
ip_address() is/should be/must be deprecated in favor of EITHER injecting a request (preferred) or calling Drupal::request(). (Or, for now, Drupal::service('request'), for which the only difference is whether or not you see a docblock in your IDE. That's *the only difference*.)
Given that ip_address() the function must die (because untestable functions that wrap quasi-testable shivs around a proper object are shooting yourself in the foot with a 12-gague), there's been nothing new said in this issue in over 20 comments.
Sorry if I sound cranky; I'm just really frustrated with issues that get stalled out on "but, but, but..." on conversations that I've already been in 6 times, often with the same people. :-(
There is one and only one question left for this thread: Do we wait for #1957156: Add a request() method to \Drupal to be commited and then use Drupal::request(), or do we commit #57 now with Drupal::service('request') and change it later if we feel like it?
There is no other question here.
Comment #82
webchickThe former.
Comment #83
Crell CreditAttribution: Crell commentedThat's one way of deciding it. :-)
New patch takes #57 and moves it over to Drupal::request(). While I was at it, I also switched the tests that effulgentsia identified to just use 127.0.0.1 (which is a good change). Grepping found only a single other instance of Drupal::service('request') so far, in Views, so I went ahead and removed that while I was at it.
Interdiff is against #57.
Comment #84
Damien Tournoud CreditAttribution: Damien Tournoud commentedI never said that. I specifically mentioned accessing the current request from the side, which is the equivalent of fiddling with
$_GET
or$_POST
. If you do that, there is no caching possible at the sub-request level anymore. Of course, that's true for many of the request locals.If you are careful, nothing is impossible. But if your assumption is that everyone is being to play nice and that you can predict the outcome of everything centrally, you are designing a web framework (like Symfony), not a web platform like Drupal.
Comment #85
Crell CreditAttribution: Crell commentedDamien: The request in the DIC is updated on subrequests and when leaving subrequests. As long as you never SAVE the value you get back from Drupal::request() and always re-request it, you should always get the right one.
Comment #86
ParisLiakos CreditAttribution: ParisLiakos commentedbot is green, interdiff looks good, i ll rtbc once more:)
Comment #87
Damien Tournoud CreditAttribution: Damien Tournoud commentedYes, I know (and never said that you don't). It doesn't change anything to my point.
Comment #88
RobLoachComment #89
RobLoachComment #90
ParisLiakos CreditAttribution: ParisLiakos commentedReroll after #1950684: Mock and protect $GLOBALS['user'] in DUTB tests went in, no longer applies, but thats good since we dont need the hack in simpletest's web test anymore:)
Comment #91
ParisLiakos CreditAttribution: ParisLiakos commentedreroll now that #1939660: Use YAML as the primary means for service registration is in
Comment #93
ParisLiakos CreditAttribution: ParisLiakos commentedyeah, wrong order of arguments
Comment #94
Crell CreditAttribution: Crell commentedAnd back.
Comment #95
effulgentsia CreditAttribution: effulgentsia commentedI opened #1965990: Figure out how to integrate hooks, $request, and caching and attempted to explain my understanding of the concern. Please see if that captures it, and edit/comment there accordingly. Thanks.
Comment #96
webchickUgh. No longer applies. Again.
Comment #97
ParisLiakos CreditAttribution: ParisLiakos commentedmanually edited to account for http://drupalcode.org/project/drupal.git/blobdiff/f28861973d7ff3483a6a6f...
Comment #98
ParisLiakos CreditAttribution: ParisLiakos commentedbot is happy, back to rtbc
Comment #99
webchickOk. Committing this while it's hot!
Committed and pushed to 8.x. Thanks!
Comment #100
webchickComment #101
ParisLiakos CreditAttribution: ParisLiakos commentedChange notice here: http://drupal.org/node/1969794
Comment #102
jibranLooks great to me.