Support from Acquia helps fund testing for Drupal Acquia logo

Comments

-enzo-’s picture

chx’s picture

Component: configuration system » documentation
Status: Needs review » Needs work
Issue tags: +Configuration system

Thanks for this patch, I miss the @return values in config greatly.

Please remove the @return on castValue, it's an array or a string not a string but hopefully #1653026: [META] Use properly typed values in module configuration will fix that for once and all.

Past that the only problem I see here is the wording on get() -- I will think on it.

-enzo-’s picture

Component: documentation » configuration system
Status: Needs work » Needs review
Issue tags: -Configuration system
FileSize
5.2 KB

Leave castValue alone for now

chx’s picture

Component: configuration system » documentation
Status: Needs review » Needs work
Issue tags: +Configuration system
chx’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

This can't be right:

/**
    * Returns the name of this configuration object.
+   *
+   * @return Drupal\Core\Config\Config
+   *   The configuration object.
    */
   public function getName() {

Either the first line or the added @return is incorrect. I didn't look through the rest of the patch, but it looks like it was just generated by pasting the same @return for each function -- it needs to be done a bit more carefully. Thanks!

socketwench’s picture

Status: Needs work » Needs review
FileSize
5.19 KB

Fixed documentation on getName().

chx’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me now and thanks for the work enzo and socketwench!

webchick’s picture

Assigned: Unassigned » jhodgdon

Jennifer's got this one. :)

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

I've committed this patch, as everything in it looks good.

But the file is not still up to our coding standards... As a follow-up, would someone like to go through and fix these items:

a) Berb tense in the function first lines? For instance:

  /**
   * Dispatch a config event.
   */
  protected function notify($config_event_name) {

Should be Dispatches. There are several others that should be fixed.

b) In the get() method, the explanation should go before the @param, not between the @param and the @return, and the @see should go at the end.

c) There are also a couple of minor wording/grammar things that could be fixed, such as:

  /**
   * Sets value in this config object.

Should be "Sets a value...".

d) The castValue() method needs a @return but doesn't have one (it has two @params, and I think the second should be @return).

e) After this patch, not all the param/returns have data types. still.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned

Oh, not my issue now. :)

chx’s picture

Do not try to fix d). castValue is scheduled to be removed, it's an abomination, so please disregard that one.

socketwench’s picture

I'll try to update the patch after work today.

alexpott’s picture

Issue tags: +Novice
vladan.me’s picture

Assigned: Unassigned » vladan.me
Priority: Normal » Minor
Status: Needs work » Needs review
FileSize
1.5 KB

I have followed instructions from #10, here's report:

a) Fixed.
b) I can't really find @see, seems that was changed in meanwhile, also, about @param and explanation, that is explanation for that specific parameter, not for whole function, so maybe it should remain there?
c) Fixed.
d) Not applicable.
e) I didn't really find any @param or @return that doesn't have data type.

jhodgdon’s picture

Status: Needs review » Needs work

These changes all look pretty good! I noticed you changed "config" to "configuration", which is excellent, but missed one:

  /**
-   * Dispatch a config event.
+   * Dispatches a config event.
    */
   protected function notify($config_event_name) {

There is also one in $isLoaded, and another in the @return of isNew(), and one in the get() method... etc.

And I agree, many of the comments in #10 were fixed between when I made them and now, so they don't apply.

There are also still a couple of functions that do not have param docs: validateName(), notify()

Thanks!

vladan.me’s picture

Thank you for reviewing patch, I've made those changes, can you please have another look? I'm attaching patch&interdiff.

vladan.me’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
1.97 KB
3.04 KB
jhodgdon’s picture

Status: Needs review » Needs work

Thanks, we're just about there!
- Can you put "a" in the clear description: "Unsets *a* value"?
- validateName() does not have @param $name documentation.
- I don't think the set() method $value parameter is actually of type 'string'... can't it be other types of data?

vladan.me’s picture

- I don't think the set() method $value parameter is actually of type 'string'... can't it be other types of data?

I really don't know, who we should ask?

vladan.me’s picture

Status: Needs work » Needs review
FileSize
3.39 KB
1.02 KB

Uploading patch according to #19
I've asked on irc, and got response that "mixed" should be valid information instead of string.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! I think this is ready to get committed now.

Xano’s picture

21: DrupalConfig-2.patch queued for re-testing.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Thanks again! Committed to 8.x (minus one stray extra line).

Status: Fixed » Closed (fixed)

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