Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NROTC_Webmaster’s picture

Status: Active » Needs review

I'm not sure of the best wording for this but let me know what you think about this.

NROTC_Webmaster’s picture

jhodgdon’s picture

Status: Needs review » Needs work

This module directory is not empty any more.

I noticed that whoever created the files there have unfortunately not followed capitalization standards, with gems like:
name = Configuration manager
(in the .info file -- should be Configuration Manager).

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
1.49 KB

A couple additional changes.

NROTC_Webmaster’s picture

This time with the newline at the end of the file.

jhodgdon’s picture

Status: Needs review » Needs work

Regarding the config.module @file block, the name of the module in the .info file is "Configuration Manager", not "Configuration". This also needs to be fixed in the config.test file.

I also noticed in config.test that at least one docblock was indented the wrong amount, so you might want to check that.

I also think that probably the config.module @file block should probably not say it's a dummy unless that's the eventual plan for the config module (I'm not sure, is it??) -- my reasoning being that no one will probably bother to go back and update it later.

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
2.38 KB

The answer I go about the config.module file from heyrocker is that the ui-facing stuff will go into it.

With that in mind I have updated the patch but I'm assuming it will be updated when there is actual code for this module and until then we want to let people know that it is supposed to be empty.

jhodgdon’s picture

Status: Needs review » Needs work

I still think we should leave out the 'currently an empty file'. That will be obvious anyway, and again I think it's likely to be left alone when someone adds stuff to the file.

And "Used by the Configuration Manager module" isn't a great file description for config.module. Take a look at some of the other .module files -- I think we usually describe what the module does. Take it from the config.info file or something. :)

The rest looks good though!

NROTC_Webmaster’s picture

Status: Needs work » Needs review
FileSize
2.36 KB

Sorry about that. I was kind of lost describing an empty file and it didn't even cross my mind to go with the description.

jhodgdon’s picture

That looks good. :) I'll give it a final review (to make sure everything's documented -- what's in the patch looks good though), and commit it when I can (depending on critical/major thresholds etc.).

jhodgdon’s picture

Component: configuration system » documentation
Status: Needs review » Needs work

This issue fell off my radar due to it being in the wrong component, sorry! I mostly look at issues in the "documentation" component.

Anyway, it's been long enough and the patch no longer applies (tests have moved to different files). So this needs a re-roll...

jhodgdon’s picture

Issue tags: +Novice

I'm going through these issues and un-assigning any without activity for several weeks (although this one was inactive due to my fault...). If you still want to work on this issue, please feel free to assign it back to yourself. Thanks!

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
8.27 KB

Attach is an locally untested patch that builds on that in #9. This work includes a full review of the top level files and the Test classes.

jhodgdon’s picture

Status: Needs review » Needs work

This type of change is not a documentation clean-up and is out of scope for this issue:

class ConfigCRUDTest extends WebTestBase {
+
   public static function getInfo() {
...
}
+
 }

Actually I am not even sure whether or not we have a *code* standard that requires these blank lines. Please remove these changes from the patch and then I'll be happy to review it.

Lars Toomre’s picture

Thanks for the review @jhodgdon. Perhaps you do not recall all of http://drupal.org/node/1354#classes? In the notes at the bottom of that section, it states:

Leave a blank line between class declaration and first internal docblock, as well as before the closing brace of the class.

I believe that the change noted in #14 conforms with our documentation standard. However, I might well be wrong despite reading through our documentation standards page.

jhodgdon’s picture

Huh, well that should not be in the documentation standards. I do not know who put that there or when, but it's not documentation and is out of scope for this issue. Thanks for pointing it out, I'll remove those from the docs page (if they are a standard they should be on the OO coding standards page, not the docs standards page).

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
5.32 KB

Here is a patch that removes the line spacing at the top/bottom of the Test classes.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks! I reviewed this patch carefully... it looks pretty good!

There are some problems with the changes in config.api.php though:

+ *
+ * @return bool
+ *   A Boolean indicating the success status of synchronizing configuration
+ *   changes.
  */
 function hook_config_import_create($name, $new_config, $old_config) {

I looked at the existing core implementations of this hook, and at least one of them (image.module) is returning something other than a Boolean value. The same is true for hook_config_import_change(). I also looked at where this hook is invoked (it's function config_import_invoke_owner() in includes/config.inc) and it is actually only registering that it didn't work if the return value is === FALSE, so it doesn't require a Boolean value actually.

And then this hook:

+ *
+ * @return bool
+ *   A Boolean indicating the success of the update operation.
  */
 function hook_config_import_delete($name, $new_config, $old_config) {

Should say delete not update. And the same comment as above applies to the Boolean assumption.

jhodgdon’s picture

Issue summary: View changes
Status: Needs work » Closed (won't fix)

These issues are a lot of work with very little tangible payoff, so I'm closing the rest of them as "won't fix". Your efforts on working on this issue were appreciated... it was just my fault for starting a task that was very difficult to get right.

Let's instead put our effort into fixing and reviewing documentation that is really unclear and/or wrong, and I hope that the people who worked on these issues are not afraid to jump into a more reasonable issue!