Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement Translatable Doctrine Extension #285

Merged
merged 18 commits into from Jan 24, 2019
Merged

Implement Translatable Doctrine Extension #285

merged 18 commits into from Jan 24, 2019

Conversation

@JarJak
Copy link
Member

@JarJak JarJak commented Jan 21, 2019

  • refactor $this->fields to a name-indexed array to make field existence check much faster (simple isset instead of foreaching)
  • rename get to getField, has to hasField
  • implement Translatable instead of reinventing the wheel

First part of #151

@JarJak JarJak requested a review from bobdenotter Jan 21, 2019
src/Entity/Field.php Outdated Show resolved Hide resolved
Loading
@bobdenotter
Copy link
Member

@bobdenotter bobdenotter commented Jan 22, 2019

Works like a charm for me!

Could we change the name of the new table from ext_translations to perhaps `bolt_translations? I found some info about that here: doctrine-extensions/DoctrineExtensions#1129

Loading

@JarJak
Copy link
Member Author

@JarJak JarJak commented Jan 23, 2019

It works but it produces a lot of DB queries... I will try to optimize it, but if it won't be possible, well, there is another approach: https://github.com/KnpLabs/DoctrineBehaviors

Loading

@JarJak JarJak changed the title Implement Translatable Doctrine Extension [WIP] Implement Translatable Doctrine Extension Jan 23, 2019
@bobdenotter
Copy link
Member

@bobdenotter bobdenotter commented Jan 23, 2019

To me, "a lot of queries" is no problem. "slow queries" are. ;-)

If O look at it, i see:

schermafbeelding 2019-01-23 om 09 30 52

So, about 5% of resources are taken by doctrine now (4.3 ms / 86 ms). Of those 4.3 ms the majority happens in the "sibebar", which we'll be caching. So, i am fine with this solution.

schermafbeelding_2019-01-23_om_09_30_39

I see KnpLabs/DoctrineBehaviors is "looking for maintainers", which doesn't say anything conclusive, but it's not a good sign that it's actively maintained.

Finally, Atlantic18/DoctrineExtensions has Tree, which is something i would really, really love to have for Content, so we can make hierarchical content out of the box.

Loading

@JarJak
Copy link
Member Author

@JarJak JarJak commented Jan 23, 2019

Interesting... this is what I see:
zrzut ekranu z 2019-01-23 09-47-58
zrzut ekranu z 2019-01-23 12-15-27

Loading

@bobdenotter
Copy link
Member

@bobdenotter bobdenotter commented Jan 23, 2019

Hmm, I'm not sure if I was looking at the correct branch earlier.. Just now i tried again, and i'm getting similar numbers as you.

Looks like it does a separate query for each field that is translated. In my case it's especially the sidebar that does a lot of queries. I wonder if this is "the price we pay" for baking in i18n, or if there's something we can do about it.

I propose we keep it like this for now, then I add caching to the sidebar, and then it'll be easier to properly inspect which queries it does exactly. Is that a plan?

Loading

@JarJak JarJak changed the title [WIP] Implement Translatable Doctrine Extension Implement Translatable Doctrine Extension Jan 23, 2019
@JarJak
Copy link
Member Author

@JarJak JarJak commented Jan 23, 2019

@bobdenotter I've made db tables prefix fully configurable.

Also I had to hack EntityManager to force Doctine joining translations in queries.

Loading

@JarJak JarJak added this to the Bolt 4 alpha 2 milestone Jan 24, 2019
Copy link
Member

@bobdenotter bobdenotter left a comment

👍

Loading

src/Entity/ContentLocalizeTrait.php Outdated Show resolved Hide resolved
Loading
@JarJak JarJak merged commit 9bb05a4 into master Jan 24, 2019
1 check passed
Loading
@bobdenotter bobdenotter deleted the feature/translatable branch Jan 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants