Problem
Drupal 8 had a centralized .po file directory to prepare for l10n_update feature inclusion in core. This centralized directory can have any number of files, and when a module is added or removed and when Drupal is installed, all files are imported that are for the languages enabled on the site. That is not very effective, and we should keep track of what did we import, and if anything changed and would need to be reimported again or we should leave the file alone.
Proposed solution
- Introduce a table to track file information (name, langcode, date)
- Fill in this table based on the files in the filesystem at first (when importing translations for example)
- On further automated imports (such as when a module is enabled, or a drush command is run), update the table with new files, and only import the needed files which are changed or new and are needed for the import (ie. in the languages configured on the site)
Parent issue
Comment | File | Size | Author |
---|---|---|---|
#77 | 1635084-followup.patch | 951 bytes | xjm |
#65 | track-imports-1635084-65.patch | 3.42 KB | penyaskito |
#62 | track-imports-1635084-62.patch | 3.42 KB | penyaskito |
#62 | track-imports-1635084-58-interdiff.txt | 3.08 KB | penyaskito |
#59 | interdiff-57-58.txt | 848 bytes | penyaskito |
Comments
Comment #1
Gábor HojtsyBetter title possibly.
Comment #2
webflo CreditAttribution: webflo commentedComment #3
Gábor HojtsyDocument $force in phpdoc.
Line too long.
This is bad but is being worked on elsewhere. Document that NULL is success. Ha.
*The* filepath.
It is strange we are using an entity for this. I understand it is not saved as a file entity, but it looks strange. *However* this is pre-existent to this patch *and* is not in the scope for the patch.
. missing at the end.
No download information here, right? Not even "project" tranlations. Its just .po files.
"File import status information for interface translation files"
something like that maybe.
?
You are using this to store the timestamp of the file itself from the point when it was last imported. Comment not consistent with usage.
TBD :)
Update functions should not use API calls. Copy the whole schema definition all over again.
Also needs tests.
Comment #4
webflo CreditAttribution: webflo commentedComment #5
webflo CreditAttribution: webflo commentedFixed the docs and added tests.
Comment #7
webflo CreditAttribution: webflo commented#5: track-import-po-files-1635084-2.patch queued for re-testing.
Comment #9
webflo CreditAttribution: webflo commentedFixed locale_update_8010().
Comment #11
webflo CreditAttribution: webflo commentedComment #12
Gábor HojtsyWrong name for class if extending webtestbase.
Woot for having bulk import tests now :) We did not have before. ++
I know you don't like UI testing as much as unit testing, but since this does the import as well, we'd ideally check the locale table for translations imported as well.
Same applies to other places after the file is imported, so maybe make it a reusable method.
Whitespace bug with the comment leading whitespace.
... available files, even if they were imported before.
// The file is already imported and it did not change since the import. ...
Comment #13
svendecabooterPatch tested and seems to work as advertised.
One problem I noticed though is when a language gets deleted and later on enabled again, the .po files get no longer imported.
The entries in locales_file probably need to be deleted when a language gets deleted?
Comment #14
svendecabooterSetting back to Gabor's status :)
Comment #15
Gábor HojtsyYes, let's delete the file, since we also drop all the strings from the DB when a language is deleted.
Comment #16
webflo CreditAttribution: webflo commentedRenamed and refactored test coded. Add function to delete po files after a language is deleted.
Comment #18
webflo CreditAttribution: webflo commentedFixed typo in langauge_list.
Comment #19
webflo CreditAttribution: webflo commentedNeeds to be $file->uri.
Its more accurate to make one query per file.
Comment #20
webflo CreditAttribution: webflo commentedComment #21
Schnitzel CreditAttribution: Schnitzel commentedReviewed the patch, looks good for me, just some small tiny things
took me some time to find out that we are actually fake the timestamp in the DB instead of changing the MTime of the File (what normally happens). I would keep it that way and maybe just make a more extended Comment.
"don't it again."
what? :)
Comment #22
vasi1186 CreditAttribution: vasi1186 commentedFound some very small, minor, issues:
Does it make sense here to use format_plural() instead of t()?
I think there is something wrong with this sentence... Should it be "Remove it from file list and don't import it again."?
I think it should be "an array".
Besides these, I think the code looks pretty good after a first review.
Comment #23
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch with these minor changes.
@Schnitzel: regarding the timestamp for the mockup files, the method documentation is like this:
Do you think we should add more explanations here or where the method is called?
Comment #24
vasi1186 CreditAttribution: vasi1186 commentedComment #25
BerdirThis is IMHO wrong.
Either you base yourself on file entities and only save the fid in your custom table, or you don't but then you shouldn't create a fake entity with entity_create() either.
If you don't, just do a new stdClass() and assign the properties.
Unless it is necessary to pass it to the locale functions, but I think we didn't add type hints there?
Comment #26
BerdirComment #27
vasi1186 CreditAttribution: vasi1186 commentedAttached a patch and an interdiff.
I also rename the function locale_translate_file_entity_create() to locale_translate_file_create().
Comment #28
webflo CreditAttribution: webflo commentedJust a minor. We don't document the type for stdClass. I looked at file_load().
@return
An object representing the file.
Comment #29
webflo CreditAttribution: webflo commentedComment #30
webflo CreditAttribution: webflo commentedSorry. This was the wrong patch.
Comment #31
attiks CreditAttribution: attiks commentedduplicate, no need for the comment.
... for an array ...
An array ...
... an older file.
So if the delete of the first file fails, all other files aren't deleted either?
It's maybe better to track the status and delete the files than can be deleted.
Comment #32
webchickNot applying. :(
Comment #33
webflo CreditAttribution: webflo commentedAddressed the comments from attiks.
Comment #34
attiks CreditAttribution: attiks commentedLooking good for me
Comment #36
webflo CreditAttribution: webflo commentedtrack-import-po-files-1635084-28.patch is already committed. track-import-po-files-1635084-28.interdiff.txt as patch.
Comment #37
webflo CreditAttribution: webflo commentedNo failing tests :)
Comment #38
Gábor HojtsyAgreed, reviewed these changes in person, should be good to go.
Comment #39
attiks CreditAttribution: attiks commentedThanks to @penyaskito
coddition --> condition
We probably need a test for this ;p
New issue created #1642966: Typo: coddition instead of condition
Comment #40
webflo CreditAttribution: webflo commentedAdded a test for locale_translate_delete_translation_files(). track-import-po-files-1635084-31-regression-test.patch should fail.
Comment #41
attiks CreditAttribution: attiks commentedcan you reroll with the test in the normal patch as well?
Comment #42
webflo CreditAttribution: webflo commentedSure. Sorry i missed that.
Comment #44
Gábor HojtsyDoes not work :(
Comment #45
attiks CreditAttribution: attiks commentedMaybe we can re-order the test as follows, i think it makes it easier to read?
10 days to next Drupal core point release.
Comment #46
penyaskito#42: track-import-po-files-1635084-32.patch queued for re-testing.
Comment #48
penyaskitoPending:
* Add asserts to first check if the file was created.
* Use public:// instead of creating the path by ourselves.
* Test again and see green :-)
Comment #49
Gábor HojtsyDiscussed this more with @rfay to figure out file test failures. Ideally we should use the setting for the directory, instead of just assuming the setting as-is by default, see http://api.drupal.org/api/drupal/core%21modules%21locale%21locale.bulk.i.... So not sure public:// will apply here, since we are handling a directory that might be entirely out of the public directory based on settings. I know the tests can assume some things, but generally, the directory can be a source controlled dir somewhere not inside public or private directories.
Comment #50
penyaskitoAs discussed on IRC between @rfay and @gaborhojtsy, we could use a StreamWrapper for this, ending in something like "translations://".
For a reference, see http://api.drupal.org/api/examples/file_example%21file_example.module/fu... and http://api.drupal.org/api/examples/file_example%21file_example_session_s....
This could make our lifes easier and our tests pass.
Comment #51
Gábor HojtsyI'm not 100% positive it will make our tests pass :) Not sure we want to do that refactoring here, while in fact we are attempting to fix a bug with the deletion code that was committed. If this little refactoring gets us there that would be helpful though, since we should likely do it either way.
Comment #52
penyaskitoAdded assert checking that the file was created.
Created Drupal\locale\TranslationsStream (but not used in tests).
Tests are passing locally, let's see the testbot.
Comment #54
attiks CreditAttribution: attiks commentedwhitespace
Comment #55
penyaskitoI asked the testbot team for running tests with --verbose for debugging what is going on on testbot.
Watch #1655158: Patch failing in qa.d.org, but not locally
Comment #56
BerdirTestbot++
http://php.net/tempnam
"Creates a file with a unique filename, with access permission set to 0600, in the specified directory. If the directory does not exist, tempnam() may generate a file in the system's temporary directory, and return the name of that."
Was able to reproduce this locally, noticed that the file was created in /tmp, a simple file_prepare_directory() fixes this. Patch coming in a second.
Comment #57
BerdirWe might want to consider using $this->randomName() instead of tempfile() because that functions does, as documented, a number of weird things that can be quite confusing. An error because the file can not be written to a non-existing directory would have been much more obvious. Also, use translations:://.
However, this should at least allow the current patch to pass.
Comment #58
penyaskitoBerdir++
Use $this->randomName() and the translations stream wrapper.
Comment #59
penyaskitoOops, wrong interdiff
Comment #60
Gábor HojtsyLooks to me the streamwrappers are not involved in fixing the bug / adding the test here. Also the stream wrapper is not used widely where the translations directory is used, so if we introduce it we should apply it more widely. I think that should be a followup.
Comment #61
penyaskitoI plan to work on this.
Comment #62
penyaskitoRemoved references to StreamWrapper. A followup will come.
This should be now ready.
Comment #63
penyaskitotranslations:// follow-up: #1658842: Introduce a translations:// stream wrapper to access the .po file directory
Comment #64
Gábor HojtsyLooks good, minus one mistake :)
Minor code style problem: dots should have spaces both before/after.
Comment #65
penyaskitoI was wrong about that, thought that was only when there were literal strings.
Comment #66
penyaskitoComment #67
Gábor HojtsyGood to go, fixes the bug (explained in http://drupal.org/node/1635084#comment-6128806), adds tests to prove it works. Retitled for current fix, should be titled / categorized back when committed.
Comment #68
Damien Tournoud CreditAttribution: Damien Tournoud commentedIf this is just fixing the bug, let's not introduce a code style regression at the same time.
The only change necessary here is
coddition() >> condition()
, the rest is just unnecessary noise. The$return
just makes the code unnecessarily convoluted.Comment #69
Gábor HojtsyDamien: well, no. The previous code stopped deleting files from both the file system and DB once a file was found that was impossible to delete. The new code will delete all files and their records in the DB that are deletable, and will return FALSE if there was at least one file that was not deletable. The meaning of the return is the same, but the deletion is more accurate, less garbage leaved behind if a file was not deletable.
Comment #70
Damien Tournoud CreditAttribution: Damien Tournoud commentedMy bad.
Comment #71
Gábor HojtsyComment #72
webchickYay! Committed and pushed to 8.x. Thanks for the expanded test coverage! :)
Comment #73
Gábor HojtsyYay, thanks.
Comment #74
Gábor HojtsyComment #75
xjmThis is causing testbot fails, probably due to
randomSting()
use. Rolling a followup now.Comment #76
xjmComment #77
xjmI suspect this should fix the problem.
Comment #78
penyaskitoHad the same issue locally. If randomeString() returns anything with a slash (/), the path is invalid and it fails. Thanks for catching it, Jess.
Comment #79
webchickGrumble grumble! Thanks. :)
Committed and pushed to 8.x.
Comment #81
xjmComment #81.0
xjmupdating summary