Rest module config is named improperly and no default config is supplied. [#1858676]

Rest module currently attempts to interface with rest.yml, however config files must have not only a module namespace but a config name. This should become rest.resources.yml. Additionally, no default configuration is supplied, it is just written out on the first submission of its settings form. A default configuration file should be supplied representing the initial configuration state (even in this case where nothing should be enabled by default.)

Patch to come.

Comment	File	Size	Author
#10	1858676-rest-config-10.patch	3.15 KB	gdd
#10
#9	1858676-rest-config.patch	2.92 KB	Damien Tournoud
#9
#7	rest.config.7.patch	2.34 KB	sun
#7
#4	1858676-rest-config-4.patch	2.34 KB	gdd
#4
#1	1858676-rest-config-1.patch	1.71 KB	gdd
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

gdd

he/him

English

Portland, OR

CreditAttribution: gdd commented 5 December 2012 at 02:22

Status:

Active

» Needs review

File	Size
1858676-rest-config-1.patch	1.71 KB

Woop!

Comment #2

tstoeckler

he/him

German

Essen, Germany

CreditAttribution: tstoeckler commented 5 December 2012 at 02:34

+++ b/core/modules/rest/config/rest.settings.yml
@@ -0,0 +1 @@
+resources: null

+++ b/core/modules/rest/rest.admin.inc
@@ -26,7 +26,7 @@ function rest_admin_form($form, &$form_state) {
+  $enabled_resources = config('rest.settings')->get('resources') ?: array();

Won't the call to config return 'null' as a string, and, hence, cast to TRUE? I think we could just put an empty array directly in the default config and then kill the ternary completely. Considering that I've not spent that much time knee-deep in CMI yet, I'm probably missing something but I wanted to bring it up just in case.

Comment #3

5 December 2012 at 03:06

Status:

Needs review

» Needs work

The last submitted patch, 1858676-rest-config-1.patch, failed testing.

Comment #4

gdd

he/him

English

Portland, OR

CreditAttribution: gdd commented 6 December 2012 at 15:52

Status:

Needs work

» Needs review

File	Size
1858676-rest-config-4.patch	2.34 KB

It helps if I convert the tests as well. This passes locally for me now.

As far as the YAML, this string null does actually convert to the value NULL as long as it isn't quoted, however I do agree that an empty array would probably be a better default value. I'm about to run but I'll get to that in a followup.

Comment #5

6 December 2012 at 16:41

Status:

Needs review

» Needs work

The last submitted patch, 1858676-rest-config-4.patch, failed testing.

Comment #6

xjm

she/her

English

CreditAttribution: xjm commented 6 December 2012 at 20:18

I downloaded this patch and issue to look at offline.

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 22 December 2012 at 14:52

Status:

Needs work

» Needs review

File	Size
rest.config.7.patch	2.34 KB

The only reason for the test failures seems to be the bogus default value for resources in the default config file.

Comment #8

22 December 2012 at 15:36

Status:

Needs review

» Needs work

The last submitted patch, rest.config.7.patch, failed testing.

Comment #9

Damien Tournoud CreditAttribution: Damien Tournoud commented 2 January 2013 at 23:08

Status:

Needs work

» Needs review

File	Size
1858676-rest-config.patch	2.92 KB

Just keeping the ball rolling. The config call in Drupal\rest\EventSubscriber\RouteSubscriber was not converted.

Comment #10

gdd

he/him

English

Portland, OR

CreditAttribution: gdd commented 3 January 2013 at 19:57

File	Size
1858676-rest-config-10.patch	3.15 KB

Last patch was missing the default config file.

Comment #11

sun

German

Karlsruhe

CreditAttribution: sun commented 3 January 2013 at 20:38

Thanks + ouch @ #9... The good thing about procedural wrappers is that we can consistently grep the entire code base for existing instances — the more stuff we inject, the more inconsistent + ungreppable becomes our code base. :-/ I hope we'll be able to address this somehow prior to release.