API page: https://api.drupal.org/api/drupal/core%21core.api.php/group/config_api/8
This page is supposed to give you the basics of the Configuration API.
It was apparently never updated after the getEditable() requirement went in (if you want to alter config, you need to call getEditable() on the config object because the default config object you get is not editable).
So the examples involving changing/saving configuration in the Simple Configuration section of this page need to be updated to use code similar to what you see on
https://www.drupal.org/node/1809490
Should be a fairly straightforward Novice project I think...
Comment | File | Size | Author |
---|---|---|---|
#56 | interdiff-2658412-46-54.txt | 1.25 KB | leolandotan |
#56 | config-api-getEditable-2658412-54.patch | 2.48 KB | leolandotan |
#49 | interdiff-2658412-46-49.txt | 1.25 KB | leolandotan |
#49 | config-api-getEditable-2658412-49.patch | 16.92 KB | leolandotan |
#46 | interdiff-2658412-41-46.txt | 1.33 KB | leolandotan |
Comments
Comment #2
miteshmapHere is the initial attempt, I hope it's in a right direction. :)
Comment #3
jhodgdonThanks for the patch!
But this patch actually doesn't fix the problems with the existing code sample from that topic:
This part of the code sample will not work, because above $config was created using:
Also, although the text here is useful, it is much more wordy than what we aim for in the topics for api.drupal.org (which is why we also have documentation pages on drupal.org that can have many more details and be more tutorial-like). What we want in the topic/@defgroup is just the bare minimum information.
So. Instead of this patch, can we:
a) Take out the incorrect part of the code that is currently in the topic (which I just mentioned in this comment), which won't work (the part with -> set and -> save methods on a non-editable config object).
b) Add a *small* explanation (like one sentence, and we don't really need the "mutable" terminology I think) about editable config objects. Then make a small code block with one line to get the editable config object and then put in those two -> set and -> save lines that were already in the topic, but now they will actually work.
c) I think you're right that adding something about delete is also useful, but it could just be one sentence of explanation and about 2 lines of code, I think?
d) Put (b) and (c) into the existing "Simple configuration" section of the topic rather than making new sections.
Thanks!
Comment #4
mikemiles86Comment #5
rakesh.gectcrComment #6
rakesh.gectcrComment #7
alvar0hurtad0Working on this.
Comment #8
alvar0hurtad0Too much documentation tasks for my poor english.
Comment #9
zweishar CreditAttribution: zweishar as a volunteer commentedReworked initial commit from @miteshmap based on feedback from @jhodgdon.
Comment #10
zweishar CreditAttribution: zweishar as a volunteer commentedComment #11
cilefen CreditAttribution: cilefen commentedWhy throw this example of a settings tree away? Let's use this example in the read-only case then write one of the same settings please.
This section makes no sense here now.
random space
Should the entire \Drupal\Core\Config\Config class path be used here?
Comment #12
thisisit CreditAttribution: thisisit commentedComment #13
thisisit CreditAttribution: thisisit at Sanganak Technologies LLP commentedApplied the first three changes mentioned by cilefen in #11. Didn't understand fourth point mentioned.
Comment #14
naveenvalechaExtra space.
Comment #15
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #16
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHi,
Here I removed the random space and added the \Drupal\Core\Config\Config class to the getEditable() method.
Thanks!
Comment #17
cilefen CreditAttribution: cilefen commentedThis line exceeds 80 columns.
I do not understand how these two sentences fit together.
This @code block is not closed and it seems in the wrong place.
Comment #18
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThank you for the review @cilefen. Will work on this.
Comment #19
rakesh.gectcrComment #20
cilefen CreditAttribution: cilefen commentedThe same thing is said twice here in different ways. I call ::getEditable() or create and object of class Config. Which is it? (I know the answer, but developers reading this will be confused).
Comment #21
lokapujyaIsn't this one big singualar code block?
Comment #22
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks for the additional patch and reviews.
@cilefen, I have tried to minimize the redundancy of the said class. I hope it's better.
@lokapujya, I have merged the last line of code into the previous code block.
Hope everything are in order. Thanks!
Comment #23
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHere is the interdiff for comment #22.
Comment #24
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #25
lokapujyaFor next patch, please follow comment #3 (specifically part b.). Use https://www.drupal.org/node/1809490 as a reference.
The idea is that the first part doesn't actually NEED to call getEditable() since nothing is saved. It just does:
$config = \Drupal::config('mymodule.foo');
The edit and save come after:
$config = \Drupal::service('config.factory')->getEditable('mymodule.foo');
(the call to getEditable(), edit(), and save() can maybe be in it's own code block with one line of explanation text before it.)
Comment #26
lokapujyaThis code example doesn't have the .yml example that goes with it from the API docs. Not sure what to do about that.
Comment #27
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #28
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThank you for the review and feedback @lokapujya!
Things done:
Comment #29
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #30
lokapujyaAlmost there.
repeated 3 times. If the first code block shows how to get an editable config object (with the "$config = "), should the next 2 code blocks assume you've already done that?
should be @endcode
This sentence doesn't sound right. I think we can take out the new text from "For the system..." down to and including this line. Also we can remove the "In this example..." sentence. Those maybe belong in the tutorial but not in the code comments?
Comment #31
harsha012 CreditAttribution: harsha012 as a volunteer and at Red Crackle commentedAdded patch as per comment #30
Comment #32
lokapujyaThanks! That fixed #30.1 and #30.2. The interdiff was not really an interdiff, it was the same as the patch.
#30.3 still needs work.
This sentence still doesn't make sense. a.) "For the system performance" is repeated from a sentence above, b.) we are not unsetting the previous code block, c.) it's not configuration that follows (it's code.) We could say, "We can unset configuration with the following code:"
Or, Perhaps we just go with what's on the doc page?:
Comment #33
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedI have tried to work on the explanations to keep them a bit shorter and direct to the point to address comment #30 item 3 and the feedback on #32.
Thanks!
Comment #34
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #35
lokapujyaLooks good to me. RTBC for me, but let's get a last look from someone else.
Comment #36
jhodgdonWell, it's definitely better than the last time I looked at it, and thanks for all the patches and reviews!
However, it still needs some reworking before I would be ready to add it to the docs:
This change is incorrect.
If you want to *change* config, you have to do what the part of the patch added below does to get a getEditable(), not this.
So you need to change these 3 lines back to what they were.
I think here we should also take out the "and setting" part, because although yes you can set things, you can't save the changes, which kind of defeats the purpose. So can we just say "getting" here and take out "and setting"?
So here I think this needs a bit more explanation that we are starting over. Something like:
The Config object that was obtained and used in the previous examples does not allow you to change configuration. If you want to change configuration, you will instead need to...
Before @code samples, end the line in :
Please check on this throughout the patch.
Please use the same example as in the code above, of 'mymodule.foo' so that it all is similar and it does not depend on a particular Core config to be defined. Also I don't really think we need both an array and a non-array set() example. One is enough. This is supposed to be bare minimum docs, not comprehensive.
If you do use an array example, please use the [] array notation not array(), which we are gradually getting rid of, and try to keep things compact (like we don't really need this local variable when the array value can be passed into set() directly).
????? TO the *file system*??? No. It is not saved to the file system. Just say "Save the configuration." or something like that.
Again, please use the mymodule.foo example that was already defined for this section, rather than introducing a new one.
And... Really, adding all of this stuff about delete() and clear() is out of scope for this issue, and adds too much bulk to this page IMO. We should just be giving the bare minimum in api.drupal.org topics. and linking to drupal.org pages for more details. So please take that stuff out. Thanks.
Comment #37
lokapujyaWill work on #36.
Comment #38
lokapujyaAddressed all points in #36.
Comment #39
lokapujyaOops, this should be 'bar', not 'bar.boo'.
Comment #40
jhodgdonAlmost, thanks!
This still needs to say 'mymodule.foo' here, not 'system.performance'.
This doesn't make sense to me. WE clear bar.boo, then get bar.boo, then the text below says it still contains something? Something is wrong here.
Oh, yeah, you figured that out in #39. Needs fixing.
Comment #41
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks @lokapujya and @jhodgdon. Here I have applied the changes specified on #39 and #40.
Comment #42
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #43
lokapujyaThank You @leolando.tan too! Nice patch.
Comment #44
jhodgdonLooks good now, thanks all!
Comment #45
catchThis should be'Individual configuration values' as below I think.
Comment #46
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThank you for the reviews and feedback! I also understood what @catch is referring to. So I:
Here I have included an updated patch and interdiff file.
Comment #47
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #48
jhodgdonQuick question:
Why are we deleting this blank line? It seems like a new paragraph here is a good idea.
Also... I don't think I like the rearrangement here. The save() docs got moved to the end, so it's way far away from the set() call and could be missed. Also, the clear() section already talks about save(). Please put it back more like it was.
Comment #49
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks for the review @jhodgdon!
Here I have added back the blank line to add the paragraph. My bad I didn't notice the other save() chained with the clean() method. So I placed placed the codes back to where they were and just added some additional details to the first "individual configuration" paragraph. Hope this makes more sense.
Thanks!
Comment #50
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #51
jhodgdonThe patch looks fine, thanks! Except that most of the changes in the patch are changes that seem to be for other issues. Please make a new patch that only contains the changes for this issue. Thanks!
Comment #52
jhodgdonAlso setting this to 8.2.x, with backport tags.
Comment #53
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedOh my! My bad. I'll fix this. Should I still branch out of 8.0.x for the patch that I'll submit?
Comment #54
cilefen CreditAttribution: cilefen commentedI asked @xjm for details, and she pointed me to https://groups.drupal.org/node/508968. For this one, it is marked for backport, so make the patch against 8.2.x please.
Comment #55
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedThanks for the confirmation @cilefen.
Comment #56
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedHere I'm working with the patch from #46 as #49's patch is wrong. So please neglect #49.
I have applied patch from comment 46 to 8.2.x and was applied cleanly, applied the recommendations of @jhodgdon from comment #48. Hope everything is in order.
Sorry my patch comment number is not right. :( If it needs to be corrected, I'd be willing to fix it.
Thanks!
Comment #57
leolandotan CreditAttribution: leolandotan as a volunteer and at Promet Source commentedComment #58
jhodgdonPatch numbers are, in my opinion, not very important that they exactly match the comment number, so don't bother to make a new one just for that!
Anyway... this patch looks fine now, thanks!
Comment #59
alexpottCommitted aff19bd and pushed to 8.0.x, 8.1.x and 8.2.x. Thanks!