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.

Files: 
CommentFileSizeAuthor
#97 drupal-remove_ip_address-1847768-97.patch33.58 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,180 pass(es).
[ View ]
#93 drupal-remove_ip_address-1847768-93.patch33.57 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,159 pass(es).
[ View ]
#93 interdiff.txt489 bytesParisLiakos
#91 drupal-remove_ip_address-1847768-91.patch33.57 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#90 drupal-remove_ip_address-1847768-90.patch33.94 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 54,093 pass(es).
[ View ]
#83 1847768-ip_address.patch34.64 KBCrell
PASSED: [[SimpleTest]]: [MySQL] 54,056 pass(es).
[ View ]
#83 interdiff.txt10.63 KBCrell
#78 drupal-remove_ip_address-1847768-78.patch27.14 KBeffulgentsia
PASSED: [[SimpleTest]]: [MySQL] 53,980 pass(es).
[ View ]
#78 interdiff.txt10.61 KBeffulgentsia
#65 ip_address-1847768-64.patch33.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 53,924 pass(es).
[ View ]
#65 interdiff.txt780 bytestim.plunkett
#57 drupal-remove_ip_address-1847768-57.patch33.21 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,628 pass(es).
[ View ]
#57 interdiff.txt1.96 KBParisLiakos
#55 drupal-remove_ip_address-1847768-55.patch32.9 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,628 pass(es).
[ View ]
#54 drupal-remove_ip_address-1847768-54-FAIL.patch32.91 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,607 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#54 drupal-remove_ip_address-1847768-54.patch32.9 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,642 pass(es).
[ View ]
#54 interdiff.txt5.07 KBParisLiakos
#49 drupal-remove_ip_address-1847768-48.patch29.02 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,394 pass(es).
[ View ]
#49 interdiff.txt1.65 KBParisLiakos
#47 drupal-remove_ip_address-1847768-46.patch28.93 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 53,148 pass(es), 1 fail(s), and 1 exception(s).
[ View ]
#47 interdiff.txt2.21 KBParisLiakos
#45 drupal-remove_ip_address-1847768-45.patch27.45 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 52,052 pass(es), 21 fail(s), and 1 exception(s).
[ View ]
#45 interdiff_.txt1.82 KBParisLiakos
#42 drupal-remove_ip_address-1847768-42.patch28.42 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,268 pass(es).
[ View ]
#42 interdiff.txt5.36 KBParisLiakos
#38 drupal-remove_ip_address-1847768-38.patch29.55 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,070 pass(es).
[ View ]
#38 interdiff.txt6.45 KBParisLiakos
#35 drupal-remove_ip_address-1847768-35.patch26.67 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 53,042 pass(es).
[ View ]
#33 drupal-remove_ip_address-1847768-32.patch25.21 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 52,812 pass(es), 323 fail(s), and 0 exception(s).
[ View ]
#31 drupal-remove_ip_address-1847768-30.patch26.18 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 51,458 pass(es), 346 fail(s), and 10 exception(s).
[ View ]
#29 drupal-remove_ip_address-1847768-29.patch21.56 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#29 interdiff.txt1.43 KBParisLiakos
#25 drupal-remove_ip_address-1847768-25.patch22.26 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] 7,530 pass(es), 608 fail(s), and 0 exception(s).
[ View ]
#23 drupal-remove_ip_address-1847768-23.patch22.26 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#24 drupal-remove_ip_address-1847768-24.patch22.23 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#19 drupal-remove_ip_address-1847768-19.patch21.79 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#16 drupal-remove_ip_address-1847768-16.patch21.31 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#14 drupal-remove_ip_address-1847768-14.patch20.96 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Title:Move ip_address() into a serviceRemove ip_address()

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

Do they have any reverse proxy support then?

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.

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.

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

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

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.

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

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.

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

Assigned:Unassigned» cgreaten

@cgreaten are you working on this?

Assigned:cgreaten» Unassigned

Status:Active» Needs review
StatusFileSize
new20.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

lets see..

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new21.31 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

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

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

Status:Needs work» Needs review
StatusFileSize
new21.79 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

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

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?

Assigned:Unassigned» ParisLiakos

picking this up again

StatusFileSize
new22.26 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

Status:Needs work» Needs review
StatusFileSize
new22.23 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Made the hack prettier to read

StatusFileSize
new22.26 KB
FAILED: [[SimpleTest]]: [MySQL] 7,530 pass(es), 608 fail(s), and 0 exception(s).
[ View ]

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.

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.

Status:Needs work» Needs review
StatusFileSize
new1.43 KB
new21.56 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

lets see if this fixes simpletest

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new26.18 KB
FAILED: [[SimpleTest]]: [MySQL] 51,458 pass(es), 346 fail(s), and 10 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new25.21 KB
FAILED: [[SimpleTest]]: [MySQL] 52,812 pass(es), 323 fail(s), and 0 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new26.67 KB
PASSED: [[SimpleTest]]: [MySQL] 53,042 pass(es).
[ View ]

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

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

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new6.45 KB
new29.55 KB
PASSED: [[SimpleTest]]: [MySQL] 53,070 pass(es).
[ View ]

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

<?php
    $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

Status:Needs review» Reviewed & tested by the community

Yay!

Status:Reviewed & tested by the community» Needs work

<?php
--- 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.

<?php
--- 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.

Assigned:ParisLiakos» Unassigned

Status:Needs work» Needs review
StatusFileSize
new5.36 KB
new28.42 KB
PASSED: [[SimpleTest]]: [MySQL] 53,268 pass(es).
[ View ]

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

Thanks! This starts to look great.

<?php
+    // 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?

<?php
+    // 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()?

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.

StatusFileSize
new1.82 KB
new27.45 KB
FAILED: [[SimpleTest]]: [MySQL] 52,052 pass(es), 21 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.21 KB
new28.93 KB
FAILED: [[SimpleTest]]: [MySQL] 53,148 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.65 KB
new29.02 KB
PASSED: [[SimpleTest]]: [MySQL] 53,394 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Looks really good to me now.

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

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.

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new5.07 KB
new32.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,642 pass(es).
[ View ]
new32.91 KB
FAILED: [[SimpleTest]]: [MySQL] 53,607 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

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

StatusFileSize
new32.9 KB
PASSED: [[SimpleTest]]: [MySQL] 53,628 pass(es).
[ View ]

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

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

StatusFileSize
new1.96 KB
new33.21 KB
PASSED: [[SimpleTest]]: [MySQL] 53,628 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

Huzzah, PHPUnit tests!

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

Why not

<?php
function ip_address() {
 
Drupal::service('request')->getClientIP();
}
?>

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

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

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

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new780 bytes
new33.3 KB
PASSED: [[SimpleTest]]: [MySQL] 53,924 pass(es).
[ View ]

For your consideration.

Status:Needs review» Reviewed & tested by the community

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.

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.

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

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.

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

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

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

<?php
$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.

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?

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.

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.

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

Status:Postponed» Needs review
StatusFileSize
new10.61 KB
new27.14 KB
PASSED: [[SimpleTest]]: [MySQL] 53,980 pass(es).
[ View ]

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.

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

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?

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.

Status:Needs review» Needs work

The former.

Status:Needs work» Needs review
StatusFileSize
new10.63 KB
new34.64 KB
PASSED: [[SimpleTest]]: [MySQL] 54,056 pass(es).
[ View ]

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.

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.

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.

Status:Needs review» Reviewed & tested by the community

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

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.

Issue tags:+needs backport to D7

Issue tags:-needs backport to D7

StatusFileSize
new33.94 KB
PASSED: [[SimpleTest]]: [MySQL] 54,093 pass(es).
[ View ]

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

StatusFileSize
new33.57 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Reviewed & tested by the community» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new489 bytes
new33.57 KB
PASSED: [[SimpleTest]]: [MySQL] 54,159 pass(es).
[ View ]

yeah, wrong order of arguments

Status:Needs review» Reviewed & tested by the community

And back.

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.

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

Ugh. No longer applies. Again.

Status:Needs work» Needs review
StatusFileSize
new33.58 KB
PASSED: [[SimpleTest]]: [MySQL] 54,180 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

bot is happy, back to rtbc

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!

Priority:Normal» Critical

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

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

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.