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.
* Notice: Undefined property: stdClass::$filepath in file_delete() (line 720 of /var/www/drupal/includes/file.inc).
* Notice: Undefined property: stdClass::$fid in file_delete() (line 721 of /var/www/drupal/includes/file.inc).
How to replicate:
- enable locale
- disable locale
- uninstall locale
Comment | File | Size | Author |
---|---|---|---|
#33 | locale-347288-33.patch | 10.15 KB | plach |
#29 | locale-347288-29.patch | 10.03 KB | plach |
#27 | locale-347288-27.patch | 10.04 KB | plach |
#25 | locale-347288-25.patch | 7.04 KB | plach |
#22 | locale-347288-22.patch | 6.73 KB | plach |
Comments
Comment #1
swentel CreditAttribution: swentel commentedEasy fix.
Comment #2
Damien Tournoud CreditAttribution: Damien Tournoud commentedCould we add a test for the uninstall feature? Several core modules have explicit uninstall code... we should test those!
Comment #3
plachThe issue was a little more complex as the uninstall hook did not clear the variables; this caused a couple of further problems:
drupal_init_language
function attempted to load the language data from thelanguages
table because of thelanguage_count
variable dirty value:Moreover the function used to delete the javascript translation files was
file_delete
instead offile_unmanaged_delete
, which caused the notices (the patch fixes this also inlocale.module
).@Damien Tournoud: [Changing status to CNR] I agree on the uninstall test, but that should be a separate task IMHO.
Comment #4
plachthe patch...
Comment #6
plachpatch fixed
Comment #7
alexanderpas CreditAttribution: alexanderpas commentedstill needs a test... otherwise, looks nice!
Comment #8
plachHere is the patch and its test.
There are two test scenarios:
Locale
is uninstalled either with English as default UI language or with French.Obviously the patch should pass both, before the patch there were two different kinds of failures.
Comment #9
Dries CreditAttribution: Dries commentedThis is starting to look good. The code comments in the test need a bit of work to confirm with the coding standards:
// Start comments with a capital letter and end with point.
Comment #10
plachSorry, my bad, comments fixed.
Comment #11
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis all looks great, thanks plach.
Please:
- document the member variable $langcode
- use setUp() rather than __construct() to modify its value. __construct is not a well-defined interface, while setUp() is.
Comment #12
plachI tried to make the change you suggested but this way I don't have the
$langcode
available inLocaleUninstallTest::getInfo
. I could set the$langcode
value directly there but I don't like this solution very much.Any advice?
Comment #13
Damien Tournoud CreditAttribution: Damien Tournoud commented@plach: you can implement getInfo() explicitly. Our coding standard for the tests (http://drupal.org/node/325974) mandates this anyway.
Comment #14
plachSorry, did not mean to change the status.
Comment #15
plachI moved the initialization of
$langcode
insetUp
and implementedgetInfo
without using its value.Comment #16
plachSome further code polishing.
Comment #17
plachAfter reading http://drupal.org/node/325974#comment-1116106 I decided to clean-up the whole
locale.test
file.I have a couple of doubts:
Comment #18
nedjoComment #20
plachRerolled patch
Comment #21
plachComment #22
plachRerolled again.
The patch won't pass all the tests because the ones introducted in #361683: Field API initial patch are failing.
Comment #23
Damien Tournoud CreditAttribution: Damien Tournoud commentedThe patch and the tests looks very good. Thanks for the clean-up plach.
Comment #24
Dries CreditAttribution: Dries commentedThe patch looks good, minus one little point. From locale_uninstall:
+ // Switch back to English to avoid problems with t().
I'd like to see us document this a bit better. What exactly is the problem with t()? I assume we need to reset the global $language object. I spent about 8 minutes trying to figure out where we'd run into a problem but could not find it. It would be good if we were a tiny bit more explicit in the code comments. (Update: I now realize it is somewhat explained in comment #3.)
In the documentation, we seem to mix 'javascript' and 'JavaScript'.
Comment #25
plachNot resetting the language may end in
locale()
to be called during the uninstall process, which may lead to queries on just dropped tables.Hope this is now more clear
(also set "JavaScript" in comments where needed)
Comment #26
alexanderpas CreditAttribution: alexanderpas commentedlooking good!
Comment #27
plachI have just realized that the JS translation files were not actually deleted, fixed locale_uninstall and added a test for this.
Morever I missed some locale variables, also this should be fixed.
Comment #29
plachThis one should pass the tests.
Comment #30
zroger CreditAttribution: zroger commentedSummary of current patch:
- Errors caused while deleting translated javascript files are fixed. Now using file_unmanaged_delete() rather than file_delete().
- The same fix was applied to _locale_rebuild_js() in locale.inc.
- Cleanup/delete locale variables on uninstall
- During uninstall, use drupal_init_language() to force language back to 'en', since successive calls to t() might cause lookups on the removed tables.
- Added tests for uninstalling the locale module.
- Cleaned up the whole locale.test file according to http://drupal.org/node/325974#comment-1116106.
Reviewed and looks good to me.
Comment #31
stella CreditAttribution: stella commentedI tested this and it works great, so rtbc from me too.
Comment #32
nedjoTalked with webchick. She's basically happy with the patch but would like to see some more documentation about the unusual last test.
This test has a setUp() method but - unusually - no tests of its own.
This is true because it inherits the test in the class it extends, LocaleUninstallFunctionalTest. In setUp() it sets a new UI language ('fr') and reruns the test.
Draft additional documentation:
Comment #33
plachAnd here it is.
Comment #34
nedjoLooks good, I think that takes care of the issue.
Comment #35
webchickGreat work on this! Thanks for the clarifying comment; I really think that'll help the next person who comes across this to understand what's happening.
Committed to HEAD. Awesome job! :)
Comment #36
plach@webchick: thank you :)