Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
The statistics module settings (admin/config/system/statistics) need to be converted to use the new configuration system.
Tasks
- Create a default config file and add it to the statistics module.
- Convert the admin UI in statistics.admin.inc to read to/write from the appropriate config.
- Convert any code that currently accesses the variables used to use the config system.
- Write an upgrade path from D7 -> D8 to migrate existing data to the new system and drop the appropriate variables.
Patch attached that covers the first 3 points. Waiting for #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system to cover the upgrade.
Comment | File | Size | Author |
---|---|---|---|
#71 | 1497310-71.patch | 21.4 KB | kbasarab |
#71 | interdiff.txt | 574 bytes | kbasarab |
#70 | 1497310-69.patch | 21.3 KB | kbasarab |
#70 | interdiff.txt | 921 bytes | kbasarab |
#67 | 1497310-67.patch | 20.85 KB | kbasarab |
Comments
Comment #2
pcambraTests fixed, resubmitting
Comment #3
marcingy CreditAttribution: marcingy commentedShould we be renaming variables such as statistics_enable_access_log to enable_access_log because effectively we are repeating data because the configuration is now namespace. I think we need to come up with a general agreement on this as it seems to be a question that is commonly being asked.
Also there is a call to update_variables_to_config which is not yet committed #1497132: Provide a generalised function to update variables from Drupal 7 to Drupal 8's configuration management system so this issue needs to be postponed on that going in.
Can't this
simply be
As NULL will evaluate as FALSE.
Comment #4
gddThis should be changed to use one $config object throughout the function to prevent unnecessary object instantiation.
Same here
I don't want to worry too much about nitpicking over the key names. It will make it easier for users moving to Drupal 8 if we have the same names, and I don't really think the extra explicitness really hurts anything. I also find it hard to get too worked up over setting a variable and checking it, rather than checking the return of the function. Setting then checking is easier to read and I don't think it really hurts anything.
3 days to next Drupal core point release.
Comment #5
pcambraHere are some of the changes implemented.
Regarding testStatisticsSettings, I've found something strange (from my understanding), I couldn't turn
Into:
Test fails as the values aren't retrieved correctly, but if I instantiate the config for each element as the patch attached, it works.
Comment #6
gddThis is currently in need of a reroll, it no longer applies. Also
The update functions for 7.x to 8.x should be contained in a special defgroup, see system.install for an example.
12 days to next Drupal core point release.
Comment #7
webchickYoinking for a re-roll.
Comment #8
webchickLet's try this on for size. Included the @defgroup as well. Followed instructions at http://learndrupal.org/lesson/b93deb54-264f-19a4-49eb-db74bc6c2712
Comment #10
alexpottThe patch in #8 is missing the configuration XML file. Creating patches that contain new files is always a bit more tricky as git has to be told about them.
Reading http://learndrupal.org/lesson/b93deb54-264f-19a4-49eb-db74bc6c2712 there needs to be a step between 10 and 11 to review any files that have been added or removed by the patch probably using "git status" and doing "git add/rm" as appropriate.
Comment #11
webchickNuts. Thanks, Alex. I'm a little rusty. :D
Let's try this on for size!
Comment #13
webchickYour mom, testbot! :P
Comment #14
webchickComment #16
webchickWell, I give up for now. If someone else wants a shot, go for it!
Comment #17
webchickI lied. I think I got it this time. modules/statistics/statstics.php is a new file since this patch was rolled and had not been converted to the new config system.
Comment #19
webchickI should not attempt fancy git things I do not understand.
Comment #21
webchickAll right, testbot. IT IS ON.
Comment #22
webchickOk, so all of these tests are failing in the same basic way.
Without the patch, the flow goes like this:
- View node/1. There are no views shown.
- View node/1 again. It shows "Viewed 1 times."
- View node/1 again. It shows "Viewed 2 times."
So the count displayed on the node is always one behind the number of times it's actually been viewed.
With the patch, it does this:
- View node/1. There are no views shown.
- View node/1 again. It shows "Viewed 2 times."
- View node/1 again. It shows "Viewed 3 times."
So after the first view, the count displayed on the node is always the same as the number of times it's actually been viewed.
It's trivial to update the tests to account for this new behaviour, but I'm not sure if it's actually behaving as intended. I suspect not. Will go digging.
Comment #23
webchick#19: config-got-you-now-sucker.patch queued for re-testing.
Comment #25
timmillwoodI don't think the patch should be adding this into statistics.module
Comment #26
webchickI agree. That probably slipped in with the merge from pre-statistics.php. Also, switching components so this is on Tim's radar.
Comment #27
webchickAlso, found two other places in the tests where we could streamline $config->get().
Comment #29
webchickEr. Apparently not. ;) #26 it is. Woohoo!
Well, at least I learned a bunch and documented this at http://drupal.org/patch/reroll
Comment #30
webchickComment #31
webchickComment #32
sunI'm sorry, but it looks like the existing config conversions totally set a wrong standard for configuration object key names.
If we don't fix that immediately, then we'll have to convert everything once more :(
So here's the thing:
'enable_access_log'
+'flush_accesslog_timer'
should be rewritten/re-keyed into'access_log.enabled'
+'access_log.flush_timer'
.This means that we naturally have a parent key of 'access_log', and you're able to retrieve all related/sub setting values via
->get('access_log')
. And of course, they're nicely grouped (even more so after switching to YAML).This also applies to the block* settings, which should actually be structured one level deeper:
block.top_day.num
block.top_all.num
block.top_last.num (might even make sense to rename this one to "recent" instead of "top_last")
Comment #33
gddThere was o standard used for variable names other than "keep what they are". We should not be blocking full functional patches while we nitpick and bike shed over variable names. We have lived with these names to now, and there is no current functional need behind changing them. It is more important that these patches land than it is that they land 100% to spec with what someone finds aesthetically pleasing. Take cleanups like this to follow ups, or if you're proposing a new variable naming convention then make a new meta issue.
Comment #34
sunjesus. All I'm asking for is this. :(
Comment #35
sunFurthermore, I did not touch this one, because this variable is not configuration.
Thus, we either revert that conversion, or this patch is blocked on finding a solution for non-configuration variables in #1175054: Add a storage (API) for persistent non-configuration state
Comment #36
ZenDoodles CreditAttribution: ZenDoodles commentedConcerns from sun make this less of a novice issue.
Comment #37
webchickHello from Core Initiative Windsprints!
This patch needs to be updated for YAML. I'll go ahead and take that on, probably sometime this weekend.
Comment #38
gddSince we have now agreed on naming conventions (http://drupal.org/node/1667896) it looks like #34 is the base patch, but the failures need to be investigated. I assume it also needs a reroll to new HEAD since there have been a ton of changes since May.
Comment #39
tobiasbUpdated agains HEAD and the new naming conventions.
It seems that drupal_array_get_nested_value() is not available in cached pages, or I dont know why the tests do not work without the include_once in Config.php.
Comment #40
sunThat means we need to resolve #1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc first. :-/
Comment #41
tobiasbComment #42
aspilicious CreditAttribution: aspilicious commentedYes tis should get removed now
newline missing at the end of the file
28 days to next Drupal core point release.
Comment #43
aspilicious CreditAttribution: aspilicious commentedadding tag
Comment #44
alexpottYaml indentation is 4 spaces
This level of hierarchy looks unnecessary and block.top_* name could be improved. How about:
Needs @see statistics_settings_form_submit()
Unnecessary @see...
Comment #45
oriol_e9gOut of scope:
This need to be resolved in a separated issue and remove de t() function for the assert messages.
Comment #46
sun@alexpott: see #32
Comment #47
tobiasbUpdated agains HEAD.
Comment #48
tobiasb* added newline
Comment #49
sunThis should be using past-tense; i.e., 'enabled'.
I had to look up what this setting means and what it actually is for.
It looks like something with "expire" and "entries" (and perhaps even "cron") would make some more sense.
In other config objects, we're using 'limit' as key name for settings like this, and it looks like these 'num' settings have the purpose (the maximum limit of items/entries to show in the block).
1) Let's add some () after "statistics_cron" while touching this comment. :)
2) We don't really have a standard for referencing keys in config objects in PHP comments yet, but so far we used
config.object:key.sub-key
(i.e., delimited by a colon). A dot as delimiter is definitely bogus though.3) The new comment line exceeds 80 chars.
Let's revert all unnecessary removals of newlines in this patch, please.
The first ->set() should be on its own line :)
&$form_state always needs to taken by reference.
Personally, Statistics module is unfortunately still a black magic box for me.
I have absolutely no idea whether it would be appropriate, but based on the configuration form elements and their descriptions, I really wonder whether something along the lines of the following keys wouldn't be more concise...?
track.page.access = 1
track.content.view = 1
Mind you, I might be completely off here.
Looks like we can revert this now. :)
Comment #50
sun@alexpott just clarified his point in #44 in IRC to me:
Statistics module only provides a single block called "popular", so I was horribly mistaken there and the suggested split of block settings makes no sense. :)
Instead, the second-level key should be
block.popular
, e.g.:Comment #51
alexpottStill think...
... is better.
Since when they are set to 0 then the part of the block controlled by the configuration will not display at all. But this is just bikehedding :)... As this configuration is used to do 2 things... whether to display to information and to set the range on a db_query in
statistics_title_list()
Comment #52
kbasarab CreditAttribution: kbasarab commentedLots of updates from the last few comments. As far as 50 and 51 go I kept as limit but I could go either way on it.
Comment #54
kbasarab CreditAttribution: kbasarab commented#52: 1497310-52.patch queued for re-testing.
Comment #55
kbasarab CreditAttribution: kbasarab commentedRerunning as the last failure was failing out on LanguageUpgradePath module which this patch doesn't touch. Test was postponed originally due to HEAD issues so we'll see what happens here.
Comment #56
tobiasbThis function needs the old keys ;-)
Comment #57
kbasarab CreditAttribution: kbasarab commentedBlah. Thanks tobiasb. Totally was in the zone of changing those and forgot I was in side the update function.
Comment #59
sun#57: 1497310-57.patch queued for re-testing.
Comment #61
aspilicious CreditAttribution: aspilicious commented"error: Call to undefined function Drupal\system\Tests\Upgrade\language_negotiation_include() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php"
means the yml file is missing ;)
I'll add it...
Comment #62
aspilicious CreditAttribution: aspilicious commentedComment #63
kbasarab CreditAttribution: kbasarab commentedThanks aspilicious. Was it just the stats yaml? Not sure how that didn't end up in my patch. The language part was throwing me.
Comment #64
aspilicious CreditAttribution: aspilicious commentedyes only the .yml file :)
Comment #65
aspilicious CreditAttribution: aspilicious commentedLooks good to me... (I only added the missing .yml file, so I'm not rtbc'ing my own patch)
Comment #66
sunHm. The "timer" part in "expire_timer" sounds a bit odd...
The value appears to be something like an interval, expressed in seconds; but it is not a frequency of something being invoked repetitively (in the future), but instead, a maximum time-frame in the past, for which records are kept.
I just had a look into default.settings.php, which apparently contains a similar PHP ini setting for cookie handling and garbage collection of sessions:
Given those examples, how about
?
Comment #67
kbasarab CreditAttribution: kbasarab commentedThat makes sense Sun. Changes attached.
Comment #68
alexpottNearly there...
We need to update the statistics_exit() code comments. Replace
statistics_enable_access_log
withstatistics.settings:access_log.enabled
Comment #69
alexpottThis comment should could read something like:
Comment #70
kbasarab CreditAttribution: kbasarab commentedUpdated. Thanks alexpott.
Comment #71
kbasarab CreditAttribution: kbasarab commentedCaught at same time there alexpott. I'm loading this and I'm off for the night. Will followup in morning with any other changes.
Comment #72
alexpottThis is good to go!
Thanks to everyone for all the work!
Comment #73
Dries CreditAttribution: Dries commentedCommitted to 8.x. Thanks all.