Locale module uses the table {locale_project} to store data about translatable projects. As suggested in #1804688: Download and import interface translations #14 this is cached data and runtime information. It is better stored in a state variable.
Locale module caches all information on available translation status (time stamp of remote and/or local translation in one state() variable. Fetching remote data is a costly operation. Therefore this will scale better for large sites with multiple languages if we can store the data in individual variables and add individual expiration.
This issue will also store the human readable project name and store it with the other project data for use in the user interface. See a follow-up note in #1804702: Display interface translation status.
Comment | File | Size | Author |
---|---|---|---|
#24 | 1842362-diff-20-24.txt | 940 bytes | vijaycs85 |
#24 | 1842362-replace-locale-tab-24.patch | 15.29 KB | vijaycs85 |
Comments
Comment #1
BerdirNote that I would recommend to not use state() but directly call into the keyvaluestore and use your own collection. drupal_container()->get('keyvalue')->get('locale') or something like that. Allows you to do stuff like getAll() if necessary.
And, you can also use the keyvalue.expirable service as well, which allows you to put stuff in there that automatically expires, if needed.
Comment #2
Sutharsan CreditAttribution: Sutharsan commentedThanks for the pointers! I picked up something, but did not get into it yet.
Comment #2.0
Sutharsan CreditAttribution: Sutharsan commentedUpdated issue summary.
Comment #3
skipyT CreditAttribution: skipyT commentedComment #4
skipyT CreditAttribution: skipyT commentedI added a new service called locale.project which stores the projects in a keyvalue collection and removed the table from the schema definition.
TODO:
- write an interface for the LocaleProjectStorage class (needed because it is a service)
- clean up the code
- perhaps we shall rewrite those code parts where the project is used as an stdClass object.
- document what are we storing in the value (needed because schema['locale_project'] was removed
Comment #5
akozma CreditAttribution: akozma commentedComment #6
Sutharsan CreditAttribution: Sutharsan commentedThe setup is pretty straight forward. Some remarks:
Needs more data of course.
getAll() method used, but not declared.
Comment #7
akozma CreditAttribution: akozma commentedCreating interface and fixing some issues.
Comment #8
skipyT CreditAttribution: skipyT commentedwe aren't sure here that all the projects are loaded in the cache, we shall return just: count($this->getAll());
here we need something like:
Comment #9
akozma CreditAttribution: akozma commentedFix for calling getAll() and other small fixes.
Comment #10
akozma CreditAttribution: akozma commentedComment #12
akozma CreditAttribution: akozma commentedRe-roll.
Comment #14
BerdirFixed typo in the filename and another small error that caused noticed in the one test that I tried to run.
Comment #16
skipyT CreditAttribution: skipyT commentedrerolled the patch and fixed the last failing test
Comment #17
skipyT CreditAttribution: skipyT commentedinterdiff added
Comment #18
Gábor Hojtsy@Berdir, @Sutharsan what do you think?
Comment #19
Sutharsan CreditAttribution: Sutharsan commentedI have only two remarks:
Should be \Drupal::service().
Small correction: "Returns a list of project records."
Comment #20
Sutharsan CreditAttribution: Sutharsan commentedRerolled the patch.
Comment #21
skipyT CreditAttribution: skipyT commented@Sutharsan: thanks! We need some review here to get an RTBC :)
Comment #22
Sutharsan CreditAttribution: Sutharsan commented@skipyT, I only re-rolled, did not include the #19 comments. With that I am comfortably enough with it to RTBC the patch.
Comment #23
Sutharsan CreditAttribution: Sutharsan commentedNo response from @ocsilalala, removing assignment.
Comment #24
vijaycs85Here is #19
Comment #25
vijaycs85looks good. overall, SQL-- and Service(keyValue)++.
Do we really need name? as the name is already the *key*?
is it overkill to ask \Drupal::service('locale.project)->getAllActive() ?
As those two are minor/improvements, +1 to RTBC.
Comment #26
Gábor HojtsyPrior code has $project, new code has $data, is this doing what the prior code did?
Prior code was not an actual count, new code is an actual count, yay :)
Comment #27
vijaycs85#26.1 - Yes, it has data already for other purposes:
that's why we have
$project->
on the query.#26.2 - Yeah, it's much clear now. DX++
Comment #28
Gábor HojtsyThanks @vijaycs85 my concerns were unfounded there. While this can be improved later on, it is already a good step, yup :)
Comment #29
webchickLooks like good clean-up.
Committed and pushed to 8.x. Thanks!
Comment #31
Gábor HojtsyMore D8-like code++ Yay, thanks!
Comment #33
BerdirFollow-up: #2834580: Move locale.translation_status state key to a separate key/value collection