Closed (fixed)
Project:
Salsa Entity
Version:
7.x-1.x-dev
Component:
Code
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
9 Oct 2013 at 15:41 UTC
Updated:
14 Mar 2014 at 09:20 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
LukyLuke_ch commentedFixed the failed tests and added some simple ones for the translation (based on the event) in two different languages.
Comment #4
LukyLuke_ch commentedMore precise mock checks to prevent the simpletest failures here.
Comment #5
berdirThanks, here's a review :)
This can't implement Entity::getTranslation() because that doesn't exist :)
Needs actual documentation.
Ok, so we cache per object now, will need to do some profiling to see how many calls we add due to this, should be ok with caching, I think. However, as we have a lot of cache entries (possibly), we should probably add our own cache table.
See token_schema() and token_update_8000() I think for how to add that, then use cache_salsa instead of cache.
Second, the cache API doesn't persist locally, every time you call it, it will load the cache from the database again.
Extend it with a local cache in $this->translations or so, first check that, then fall back to cache_get(), then fall back to a salsa query.
move this to the class docblock, make this just Contains ClassName.
We need to namespace class names as long as we don't have namespaces, so make it SalsEntityIso693.
Returns (uppercase)
format for optional is: (optional) Explain what it is, defaults to X.
get() is too generic, it doesn't tell me what it does/return.
And, if we add this, then we should allow to convert in both directions. We've had use cases for that before.
toIso693() and fromIso693() maybe?
If we have it in the method, we could name the class SalsaEntityLanguageMapper or so?
property declarations should be above methods. I also think this is not complete, the salsa UI has like 20 languages in the translation popup?
Why does the filename have -3 and the class name not? (might change anyway according to suggestions above.
getInfo() should be above setUp()
By making two methods, you a) make the test twice as slow and b) don't verify that per-language caches work properly, because this is a completely different installation.
You also don't need the permission bit, as you just access the event directly.
Comment #6
berdirRe-rolled, review still needs to be updated.
Comment #7
berdirRe-roll.
1. Ok, it actually does although it's a bit different. So left that.
Everything else should mostly be addressed, including a cache table.
I also added a cache clear button to specifically clear translations and took the opportunity to convert it from a separate local task to a fieldset on the main salsa_api settings page. And cleaned up the submit callbacks, we no longer need to clear the entity_info cache or rebuild the menu, there are no settings left that can affect this.
Comment #8
berdirCommitted and pushed!