In #1615520: Add translatable config_simple_example we want to create a new example module for the config system, but to truly demonstrate the upgrade path we will need to have a similar module in D7 (suggested by rfay in comment #10 of the above-mentioned issue).

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alberto56’s picture

Status: Active » Needs review
FileSize
47.99 KB

Here is a patch

Status: Needs review » Needs work

The last submitted patch, 1630762-1-D7-config-module.patch, failed testing.

alberto56’s picture

Status: Needs work » Needs review
FileSize
20.1 KB

Here is another one.

Status: Needs review » Needs work

The last submitted patch, 1630762-3-D7-config-module.patch, failed testing.

alberto56’s picture

Status: Needs work » Needs review
FileSize
20.1 KB

oops, syntax error

rfay’s picture

Status: Needs review » Needs work

@alberto56, thanks for this! I think you're well on the way.

I wasn't able to understand your intent by just reading the code; the README.txt helped, but the code didn't really explain to me what you were doing.

Could you
1. Simplify the database-based configuration so you can just focus on the fact that it's configuration.
2. Improve the comments to explain that you're using the database table for configuration (and how it changes the behavior of the module)

Please begin the .module file with an explanation of what you're trying to do (the info you put into the README).

Please move the non-hook functions below the hook functions.

When you do the database-based configuration, please explain what it is that is happening.

Again, thanks very much for this, and sorry for the slow review!

alberto56’s picture

@rfay thanks for the feedback. I have started work on this, and will provide an updated patch when I return from my vacation mid-July.

Cheers,

Albert.

alberto56’s picture

Assigned: Unassigned » alberto56
Status: Needs work » Needs review
FileSize
24.86 KB

Hi,

Here is a new patch which takes rfay's considerations into account. Here are a few notes:

(1) "Simplify the database-based configuration": I have removed the kitten "size", leaving only the name, machine name, and color. These are all important in my opinion: the machine name is automatically derived from the name -- this is standard throughout the Drupal interface I think; and the color exists so that we can associated two (or more) pieces of data, which is what complex configuration usually does. If anyone has any concrete ideas as to how I can simplify this more, please let me know!

(2) I have added comments everywhere to explain further what I am trying to accomplish. If any passage or code appears unclear, please specify which. Specifically, @rfay if you still would like clarification on database-based configuration, please refer to precise passages.

(3) Function order: Hook functions, then non-hook functions.

(4) Added a new test to confirm that deleting one item does not delete others as well.

(5) All tests pass and have been tested.

(6) These improvements have also been included in a new version (comment #15) of the patch in #1615520: Add translatable config_simple_example

(7) Repaired an issue, and added a test, where deleting a set xyz from the complex configuration caused a erroneous message "xyz does not exist" to appear on-screen.

Cheers,

Albert.

alberto56’s picture

Status: Needs review » Active

In light of #1699440: A dedicated to module to demonstrate hook_update_N() and automatically testing upgrades and updates, I suggest setting this issue to "not fixed". This patch's reason for being was only to demonstrate the upgrade path to #1615520: Add translatable config_simple_example, but it seems clunky to me now -- the upgrade path should be demonstrated in a separate example module.

Thoughts?

Mile23’s picture

Issue summary: View changes
Status: Active » Closed (duplicate)

I'm closing this as a duplicate of #2182621: Add Migrate example

If there's something else going on here that I'm missing, please re-open this issue. Thanks.