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.

CommentFileSizeAuthor
#97 drupal-remove_ip_address-1847768-97.patch33.58 KBParisLiakos
#93 drupal-remove_ip_address-1847768-93.patch33.57 KBParisLiakos
#93 interdiff.txt489 bytesParisLiakos
#91 drupal-remove_ip_address-1847768-91.patch33.57 KBParisLiakos
#90 drupal-remove_ip_address-1847768-90.patch33.94 KBParisLiakos
#83 1847768-ip_address.patch34.64 KBCrell
#83 interdiff.txt10.63 KBCrell
#78 drupal-remove_ip_address-1847768-78.patch27.14 KBeffulgentsia
#78 interdiff.txt10.61 KBeffulgentsia
#65 ip_address-1847768-64.patch33.3 KBtim.plunkett
#65 interdiff.txt780 bytestim.plunkett
#57 drupal-remove_ip_address-1847768-57.patch33.21 KBParisLiakos
#57 interdiff.txt1.96 KBParisLiakos
#55 drupal-remove_ip_address-1847768-55.patch32.9 KBParisLiakos
#54 drupal-remove_ip_address-1847768-54-FAIL.patch32.91 KBParisLiakos
#54 drupal-remove_ip_address-1847768-54.patch32.9 KBParisLiakos
#54 interdiff.txt5.07 KBParisLiakos
#49 drupal-remove_ip_address-1847768-48.patch29.02 KBParisLiakos
#49 interdiff.txt1.65 KBParisLiakos
#47 drupal-remove_ip_address-1847768-46.patch28.93 KBParisLiakos
#47 interdiff.txt2.21 KBParisLiakos
#45 drupal-remove_ip_address-1847768-45.patch27.45 KBParisLiakos
#45 interdiff_.txt1.82 KBParisLiakos
#42 drupal-remove_ip_address-1847768-42.patch28.42 KBParisLiakos
#42 interdiff.txt5.36 KBParisLiakos
#38 drupal-remove_ip_address-1847768-38.patch29.55 KBParisLiakos
#38 interdiff.txt6.45 KBParisLiakos
#35 drupal-remove_ip_address-1847768-35.patch26.67 KBParisLiakos
#33 drupal-remove_ip_address-1847768-32.patch25.21 KBParisLiakos
#31 drupal-remove_ip_address-1847768-30.patch26.18 KBParisLiakos
#29 drupal-remove_ip_address-1847768-29.patch21.56 KBParisLiakos
#29 interdiff.txt1.43 KBParisLiakos
#25 drupal-remove_ip_address-1847768-25.patch22.26 KBParisLiakos
#23 drupal-remove_ip_address-1847768-23.patch22.26 KBParisLiakos
#24 drupal-remove_ip_address-1847768-24.patch22.23 KBParisLiakos
#19 drupal-remove_ip_address-1847768-19.patch21.79 KBParisLiakos
#16 drupal-remove_ip_address-1847768-16.patch21.31 KBParisLiakos
#14 drupal-remove_ip_address-1847768-14.patch20.96 KBParisLiakos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

Title: Move ip_address() into a service » Remove ip_address()

Symfony already have some primitive support for this in the Request object, we should just remove our implementation.

catch’s picture

Do they have any reverse proxy support then?

Damien Tournoud’s picture

Yes, they have support for Client-Ip (that we don't have) and some primitive support for X-Forwarded-For. You need to call Request::trustProxyData() for that to happen.

sun’s picture

The 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.

Crell’s picture

In 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".

catch’s picture

#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.

Damien Tournoud’s picture

We 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.

catch’s picture

Upstream PRs are fine but they need to be done before actually using the thing being PRed.

catch’s picture

Turns 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.

Crell’s picture

Our Symfony code has been re-upped, so let's get back to this issue: #1879410: Update Symfony dependency

cgreaten’s picture

Assigned: Unassigned » cgreaten
ParisLiakos’s picture

@cgreaten are you working on this?

cgreaten’s picture

Assigned: cgreaten » Unassigned
ParisLiakos’s picture

Status: Active » Needs review
FileSize
20.96 KB

lets see..

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-14.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
21.31 KB

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-16.patch, failed testing.

ParisLiakos’s picture

Installing fails cause _drupal_session_write tries to write in database an entry with NULL hostname..apparently somehow Symfony's Request 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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
21.79 KB
// @todo convert to Request::getClientIp().
'hostname' => $_SERVER['REMOTE_ADDR'],

This in _drupal_session_write() gets me past the installer at least
Also, i am almost sure that my approach with flood classes is wrong, will figure it out another day though

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-19.patch, failed testing.

Damien Tournoud’s picture

Installing fails cause _drupal_session_write tries to write in database an entry with NULL hostname..apparently somehow Symfony's Request is initialized without any parameter

Shooting in the dark here. But possibly we currently write to the session *after* the container has teared down?

ParisLiakos’s picture

Assigned: Unassigned » ParisLiakos

picking this up again

ParisLiakos’s picture

This is probably THE hack, but thats the only way i could find to make installer work

+    // Set the request in the kernel to the new created Request above
+    // so it is available to the rest of the installation process.
+    $container = $kernel->getContainer();
+    $container->enterScope('request');
+    $container->set('request', $request);

Also what should i do with flood classes?
Probably this is not ideal:

--- a/core/lib/Drupal/Core/Flood/DatabaseBackend.php
+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -37,7 +37,7 @@ public function __construct(Connection $connection) {
    */
   public function register($name, $window = 3600, $identifier = NULL) {
     if (!isset($identifier)) {
-      $identifier = ip_address();
+      $identifier = \Drupal::service('request')->getClientIP();

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

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
22.23 KB

Made the hack prettier to read

ParisLiakos’s picture

ah, 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

Status: Needs review » Needs work
Issue tags: -WSCCI

The last submitted patch, drupal-remove_ip_address-1847768-25.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +WSCCI

The last submitted patch, drupal-remove_ip_address-1847768-25.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
21.56 KB

lets see if this fixes simpletest

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-29.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
26.18 KB

Urm, patch above misses reverse proxy subscriber:/

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-30.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
25.21 KB

Ok lets see this one..still need some advice with flood classes, i am not sure whats the best way to act there

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-32.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
26.67 KB

This should go green..i had to update the request in module_enable|disable cause the kernel was being rebuilt

Crell’s picture

+++ b/core/lib/Drupal/Core/EventSubscriber/ReverseProxySubscriber.php
index a7b05f4..54a8ffa 100644
--- a/core/lib/Drupal/Core/Flood/DatabaseBackend.php

--- a/core/lib/Drupal/Core/Flood/DatabaseBackend.php
+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php

+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
@@ -37,7 +37,7 @@ public function __construct(Connection $connection) {

@@ -37,7 +37,7 @@ public function __construct(Connection $connection) {
    */
   public function register($name, $window = 3600, $identifier = NULL) {
     if (!isset($identifier)) {
-      $identifier = ip_address();
+      $identifier = \Drupal::service('request')->getClientIP();

The 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.

+++ b/core/lib/Drupal/Core/Flood/DatabaseBackend.php
index 63b19da..ff49eff 100644
--- a/core/lib/Drupal/Core/Flood/MemoryBackend.php

--- a/core/lib/Drupal/Core/Flood/MemoryBackend.php
+++ b/core/lib/Drupal/Core/Flood/MemoryBackend.php

+++ b/core/lib/Drupal/Core/Flood/MemoryBackend.php
+++ b/core/lib/Drupal/Core/Flood/MemoryBackend.php
@@ -22,7 +22,7 @@ class MemoryBackend implements FloodInterface {

@@ -22,7 +22,7 @@ class MemoryBackend implements FloodInterface {
    */
   public function register($name, $window = 3600, $identifier = NULL) {
     if (!isset($identifier)) {
-      $identifier = ip_address();
+      $identifier = \Drupal::service('request')->getClientIP();

Same here.

Other than that, this looks great!

Crell’s picture

Status: Needs review » Needs work
ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
6.45 KB
29.55 KB

Hmm 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

    $container->register('flood', 'Drupal\Core\Flood\DatabaseBackend')
      ->addArgument(new Reference('database'))
      ->addArgument(new Reference('request'));

I thought it wouldnt work, but well i dont seem to get any error and flood tests pass..so i guess it is ok

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Yay!

Damien Tournoud’s picture

Status: Reviewed & tested by the community » Needs work
--- a/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
+++ b/core/modules/simpletest/lib/Drupal/simpletest/TestBase.php
@@ -907,6 +908,9 @@ protected function prepareEnvironment() {
     unset($GLOBALS['theme_key']);
     unset($GLOBALS['theme']);
 
+    // Add request info.
+    $this->container->set('request', Request::createFromGlobals());
+
     // Log fatal errors.
     ini_set('log_errors', 1);
     ini_set('error_log', $this->public_files_directory . '/error.log');
@@ -968,6 +972,8 @@ protected function rebuildContainer() {
     // DrupalKernel replaces the container in drupal_container() with a
     // different object, so we need to replace the instance on this test class.
     $this->container = drupal_container();
+    // Add request info to the container.
+    $this->container->set('request', Request::createFromGlobals());
   }

^ This break encapsulation. We should create a dummy request instead.

--- a/core/includes/module.inc
+++ b/core/includes/module.inc
@@ -354,6 +355,8 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
       // @todo install_begin_request() creates a container without a kernel.
       if ($kernel = drupal_container()->get('kernel', ContainerInterface::NULL_ON_INVALID_REFERENCE)) {
         $kernel->updateModules($module_filenames, $module_filenames);
+        // Kernel has been rebuilt so add the request info back.
+        Drupal::getContainer()->set('request', Request::createFromGlobals());
       }
 
       // Refresh the schema to include it.
@@ -512,6 +515,8 @@ function module_disable($module_list, $disable_dependents = TRUE) {
     // Update the kernel to exclude the disabled modules.
     $enabled = $module_handler->getModuleList();
     drupal_container()->get('kernel')->updateModules($enabled, $enabled);
+    // Kernel has been rebuilt so add the request info back.
+    Drupal::getContainer()->set('request', Request::createFromGlobals());
 
     // Update the theme registry to remove the newly-disabled module.
     drupal_theme_rebuild();

^ 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.

ParisLiakos’s picture

Assigned: ParisLiakos » Unassigned
ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
5.36 KB
28.42 KB

Indeed! i think its way better now..thanks @katbailey for some hints and damien for explanation..lets see if bot agrees too

Damien Tournoud’s picture

Thanks! This starts to look great.

+    // If we have a request set it back to the new container.
+    if (isset($request)) {
+      $this->container->set('request', $request);
+    }

^ Don't we also need to start a request scope?

+    // Add a dummy request in the container.
+    $request = Request::create('http://example.com/');
+    // We need an IP when storing sessions.
+    $request->server->set('REMOTE_ADDR', '3.3.3.3');
+    $this->container->set('request', $request);
+
     // Log fatal errors.
     ini_set('log_errors', 1);
     ini_set('error_log', $this->public_files_directory . '/error.log');
@@ -968,6 +975,12 @@ protected function rebuildContainer() {
     // DrupalKernel replaces the container in drupal_container() with a
     // different object, so we need to replace the instance on this test class.
     $this->container = drupal_container();
+
+    // Add a dummy request in the container.
+    $request = Request::create('http://example.com/');
+    // We need an IP when storing sessions.
+    $request->server->set('REMOTE_ADDR', '3.3.3.3');
+    $this->container->set('request', $request);
   }

^ Do we need both? Isn't the second one already covered by the code from DrupalKernel::initializeContainer()?

Damien Tournoud’s picture

Also, 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.

ParisLiakos’s picture

wow, 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

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-45.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
2.21 KB
28.93 KB

ok, 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:)

Status: Needs review » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-46.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.65 KB
29.02 KB

that view test should be a random failure, i cannot reproduce it locally..i had to make an adjustment for the flood tests

Damien Tournoud’s picture

Status: Needs review » Reviewed & tested by the community

Looks really good to me now.

podarok’s picture

Issue tags: +Needs change record
+++ b/core/includes/bootstrap.incundefined
@@ -3061,52 +3062,6 @@ function arg($index = NULL, $path = NULL) {
- */
-function ip_address() {

this one needs change notification

+1RTBC

xjm’s picture

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs change record +Needs tests

Since 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.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
5.07 KB
32.9 KB
32.91 KB

thanks to msonnabaum for the initial hints:)
also found out that the request methods are static..thanks phpunit:P

ParisLiakos’s picture

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ReverseProxySubscriberUnitTest.phpundefined
@@ -0,0 +1,104 @@
+ * Contains \Drupal\Core\EventSubscriber\ReverseProxySubscriberUnitTest.php.

oops, that trailing .php shouldnt be there:P removed

Crell’s picture

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ReverseProxySubscriberUnitTest.php
@@ -0,0 +1,104 @@
+  /**
+   * Tests if \Symfony\Component\HttpFoundation\Request::setTrustedHeaderName()
+   * and \Symfony\Component\HttpFoundation\Request::setTrustedProxies() are
+   * called when reverse proxy settings are enabled.
+   *
+   * @param \Drupal\Component\Utility\Settings $settings
+   *   The settings object that holds reverse proxy configuration.

Shortdesc 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.)

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/ReverseProxySubscriberUnitTest.php
@@ -0,0 +1,104 @@
+  /**
+   * Mocks a \Symfony\Component\HttpKernel\Event\GetResponseEvent object
+   * and stubs its getRequest() method to return a mocked request object.

Same.

ParisLiakos’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Huzzah, PHPUnit tests!

xjm’s picture

webchick’s picture

-      'ip'          => ip_address(),
+      'ip'          => Drupal::service('request')->getClientIP(),

I 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

greggles’s picture

Why not

function ip_address() {
  Drupal::service('request')->getClientIP();
}
tim.plunkett’s picture

That, plus a nice @deprecated tag, and call it even? That's what we've done elsewhere.

Dave Reid’s picture

Can we at least deprecate the functions like this for now rather than just plain remove?

ParisLiakos’s picture

i 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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
780 bytes
33.3 KB

For your consideration.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community
Crell’s picture

It'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:

  $ip = $this->request->getClientIp();

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.

Damien Tournoud’s picture

Yes, 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.

Damien Tournoud’s picture

(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)

Dave Reid’s picture

Ok 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.

msonnabaum’s picture

Agree with #70. It's not about procedural vs DI, it's about not introducing unnecessary complexity at the API level.

ParisLiakos’s picture

Lets get #65 in and then remove ip_address() in #1957156: Add a request() method to \Drupal and be done with this issue?

Damien Tournoud’s picture

@Dave, msonnabaum: as developers, you are going to use:

$ip = $this->request->getClientIp();

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.

Dave Reid’s picture

So 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?

effulgentsia’s picture

Drupal 8 cannot ship if you still have major needs to access the request from the Drupal:: global.

Why 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.

webchick’s picture

Status: Reviewed & tested by the community » Postponed

I think we should probably postpone this on #1957156: Add a request() method to \Drupal and sort out what we want to do here.

Crell’s picture

Dave: 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?"

effulgentsia’s picture

Status: Postponed » Needs review
FileSize
10.61 KB
27.14 KB

Based 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.

Crell’s picture

+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.

effulgentsia’s picture

If we must keep ip_address() in existence for a little longer, at the very least that's what @deprecated is for.

How 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?

Drupal::service('request') is going to continue to work

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?

Crell’s picture

s/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.

webchick’s picture

Status: Needs review » Needs work

The former.

Crell’s picture

Status: Needs work » Needs review
FileSize
10.63 KB
34.64 KB

That'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.

Damien Tournoud’s picture

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.

I 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.

Crell’s picture

Damien: 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.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

bot is green, interdiff looks good, i ll rtbc once more:)

Damien Tournoud’s picture

Damien: 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.

Yes, I know (and never said that you don't). It doesn't change anything to my point.

RobLoach’s picture

Issue tags: +Needs backport to D7
RobLoach’s picture

Issue tags: -Needs backport to D7
ParisLiakos’s picture

Reroll 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:)

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-remove_ip_address-1847768-91.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
489 bytes
33.57 KB

yeah, wrong order of arguments

Crell’s picture

Status: Needs review » Reviewed & tested by the community

And back.

effulgentsia’s picture

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.

I 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Avoid commit conflicts

Ugh. No longer applies. Again.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
33.58 KB
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

bot is happy, back to rtbc

webchick’s picture

Title: Remove ip_address() » Change notice: Remove ip_address()
Status: Reviewed & tested by the community » Active
Issue tags: -Avoid commit conflicts +Needs change record

Ok. Committing this while it's hot!

Committed and pushed to 8.x. Thanks!

webchick’s picture

Priority: Normal » Critical
ParisLiakos’s picture

Status: Active » Needs review
Issue tags: -Needs change record

Change notice here: http://drupal.org/node/1969794

jibran’s picture

Title: Change notice: Remove ip_address() » Remove ip_address()
Priority: Critical » Normal
Status: Needs review » Fixed

Looks great to me.

Automatically closed -- issue fixed for 2 weeks with no activity.