Part of #1775842: [meta] Convert all variables to state and/or config systems

The reverse proxy settings need to be converted to the new configuration management system

Variables to be converted are:

* reverse_proxy
* reverse_proxy_addresses
* reverse_proxy_header

Files: 
CommentFileSizeAuthor
#28 1793102-28-convert-reverse-proxy-settings-to-cmi.patch5.79 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 46,194 pass(es).
[ View ]
#28 interdiff.txt384 byteskim.pepper
#27 1793102-26-convert-reverse-proxy-settings-to-cmi.patch5.79 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 46,200 pass(es).
[ View ]
#27 interdiff.txt708 byteskim.pepper
#26 1793102-26-convert-reverse-proxy-settings-to-cmi.patch5.95 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 46,197 pass(es).
[ View ]
#26 interdiff.txt356 byteskim.pepper
#25 1793102-25-convert-reverse-proxy-settings-to-cmi.patch5.95 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 46,052 pass(es).
[ View ]
#22 1793102-22-convert-reverse-proxy-settings-to-cmi.patch5.95 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 41,918 pass(es), 6 fail(s), and 8 exception(s).
[ View ]
#20 1793102-20-convert-reverse-proxy-settings-to-cmi.patch5.79 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]
#16 1793102-16-convert-reverse-proxy-settings-to-cmi.patch5.85 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 41,458 pass(es), 36 fail(s), and 19 exception(s).
[ View ]
#9 1793102-9-convert-reverse-proxy-settings-to-cmi.patch5.89 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 42,006 pass(es), 17 fail(s), and 22 exception(s).
[ View ]
#3 1793102-3-convert-reverse-proxy-settings-to-cmi.patch5.9 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 41,291 pass(es), 21 fail(s), and 36 exception(s).
[ View ]
#1 1793102-1-convert-reverse-proxy-settings-to-cmi.patch5.87 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 41,473 pass(es).
[ View ]

Comments

kim.pepper’s picture

Status:Active» Needs review
StatusFileSize
new5.87 KB
PASSED: [[SimpleTest]]: [MySQL] 41,473 pass(es).
[ View ]

Initial patch. I created a new config file called system.reverse-proxy.yml in the system module.

Kim

swentel’s picture

Status:Needs review» Needs work

Looks good overall to me, one tiny little nitpick though:

+++ b/sites/default/default.settings.phpundefined
@@ -398,35 +398,36 @@ ini_set('session.cookie_lifetime', 2000000);
+ * This setting is required if $conf['system.reverse-proxy']['enabled'] is ¶

Extra space after 'is' should be removed.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.9 KB
FAILED: [[SimpleTest]]: [MySQL] 41,291 pass(es), 21 fail(s), and 36 exception(s).
[ View ]

Removed trailing space and renamed config variable for consistency.

K

Status:Needs review» Needs work

The last submitted patch, 1793102-3-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1793102-3-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1793102-3-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.89 KB
FAILED: [[SimpleTest]]: [MySQL] 42,006 pass(es), 17 fail(s), and 22 exception(s).
[ View ]

Re-rolled against latest 8.x

Status:Needs review» Needs work

The last submitted patch, 1793102-9-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review

I'm getting an exception 'PDOException' with message 'SQLSTATE[08004] [1040] Too many connections' from testbot.

kim.pepper’s picture

Status:Needs review» Needs work
Issue tags:+#pnx-sprint
kim.pepper’s picture

kim.pepper’s picture

Status:Needs work» Needs review
Issue tags:-#pnx-sprint

Status:Needs review» Needs work
Issue tags:+#pnx-sprint

The last submitted patch, 1793102-9-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.85 KB
FAILED: [[SimpleTest]]: [MySQL] 41,458 pass(es), 36 fail(s), and 19 exception(s).
[ View ]

Re-rolled against latest 8.x-dev. Still no idea why these tests passed but are now failing...

Status:Needs review» Needs work

The last submitted patch, 1793102-16-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Still getting "Too Many Connections" errors.

I have run these failing tests locally and they all pass.

kim.pepper’s picture

Assigned:kim.pepper» Unassigned

Unassigning as I can't think of anything else that could cause this. :-(
K

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.79 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.install.
[ View ]

Rebased off latest 8.x

Still don't know why this is failing. Looking for someone to provide some help.

Status:Needs review» Needs work

The last submitted patch, 1793102-20-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.95 KB
FAILED: [[SimpleTest]]: [MySQL] 41,918 pass(es), 6 fail(s), and 8 exception(s).
[ View ]

update hook bump

Status:Needs review» Needs work

The last submitted patch, 1793102-22-convert-reverse-proxy-settings-to-cmi.patch, failed testing.

kim.pepper’s picture

"Too many connections" GAAAAAAAH!

kim.pepper’s picture

Status:Needs work» Needs review
StatusFileSize
new5.95 KB
PASSED: [[SimpleTest]]: [MySQL] 46,052 pass(es).
[ View ]

Re-roll

kim.pepper’s picture

StatusFileSize
new356 bytes
new5.95 KB
PASSED: [[SimpleTest]]: [MySQL] 46,197 pass(es).
[ View ]

@heyrocker says we are using 0/1 for booleans in yaml files.

kim.pepper’s picture

StatusFileSize
new708 bytes
new5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 46,200 pass(es).
[ View ]

Fixed whitespace

kim.pepper’s picture

StatusFileSize
new384 bytes
new5.79 KB
PASSED: [[SimpleTest]]: [MySQL] 46,194 pass(es).
[ View ]
larowlan’s picture

Status:Needs review» Reviewed & tested by the community

Looks good

catch’s picture

Status:Reviewed & tested by the community» Needs review

I don't think this should necessarily be converted to config at all for several reasons:

- whether there's a reverse proxy or not, and what those are, may differ between staging and production. We don't have a concept of environment-specific configuration in the config API yet, or a recommended way to handle this.

- these settings were always meant to be hard-coded in settings.php, because ip_address() is called in drupal_anonymous_user() and watchdog() - so this is adding a config() dependency very, very low down in the system.

- It's also going to add database requests where we previously had none.

Not moving this to CNW, but it needs more discussion. The alternative is to move these globals out of $conf (similar to global $databases).

Also if we do actually convert this to CMI, then it shouldn't be listed in settings.php since that's not going to be the recommended way to set it up any more.

kim.pepper’s picture

@catch, thanks for your feedback.

- whether there's a reverse proxy or not, and what those are, may differ between staging and production. We don't have a concept of environment-specific configuration in the config API yet, or a recommended way to handle this.

My understanding is that Symfony has the concept of dev and prod, but not sure if it handles multiple environments. This is handled in http://drupal.org/project/env in a similar way to Ruby on Rails, where config for each environment is checked into source control.

- It's also going to add database requests where we previously had none.

Damn. Is that the case for all config? My understanding of the config system is limited.

Kim

heyrocker’s picture

Each request to config is a file system hit when the cache is cold, and a database query when its warm. There are issues open to begin addressing this with static caching and other possibilities. But its probably going to wait until after feature freeze before it goes anywhere.

Managing contextual config differences between dev and prod will most likely be solved by contrib using the new contextual override system (same thing multilingual is using.)

I do agree that this is almost always going to be different between dev and prod, and thus probably isn't best placed in config. I think catch's suggestion of making it a hardcoded variable ala $databases is not a bad idea. Another possibility would be the state system, but that will add the same overhead that config does in terms of db queries (I think.)

kim.pepper’s picture

Status:Needs review» Closed (duplicate)
kim.pepper’s picture

Issue summary:View changes

Fixed copy/paste error