Based on #1498634: [meta] Define revision/translation SQL schema for core entities (no patch) there should be a new schema.
Comment should add data table {comment_field_data}
it's connected to {comment}
directly.
For further details read Added multilingual support to the standard entity schema
Comment | File | Size | Author |
---|---|---|---|
#55 | 1498662-comment-subject-55.patch | 46.84 KB | andypost |
Comments
Comment #1
dawehnerAdd tags
Comment #2
Gábor HojtsyRetitle based on #1658712: Refactor test_entity schema to be multilingual. Marking postponed on #1691952: Make EntityFieldQuery work with multilingual properties.
Comment #3
Gábor HojtsyComments are converted to Entity NG, so this should be fully possible now.
Comment #4
Gábor HojtsyTagging for D8MI.
Comment #5
andypostFollowing here to have less collisions with #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Comment #6
andypostAlso related #2028025: Expand CommentInterface to provide methods
Comment #7
klonos...related issues have a special, dedicated place after the d.o D7 upgrade ;)
Comment #8
andypostShould be done before beta
Comment #9
andypostInitial patch - just subject needs translation.
There's a lot conversions happen with comment queries in #2068325: [META] Convert entity SQL queries to the Entity Query API
Comment #11
andyposthm, not so much left direct queries...
Patch:
- fixed schema, finally get rid of "array-index" - close to postgres ;)
- fixed direct queries, should be combined with other entity query patches
- a hack to remove join on user table (author_name), probably needs @berdir
todo:
1) profile new queries and tune indexes
2) views integration, probably could be solved with #1740492: Implement a default entity views data handler
3) search module integration
Comment #12
andypostSuppose better to work on this issue parallel with #2068331: Convert comment SQL queries to the Entity Query API
Comment #14
andypostFiled #2292815: Remove join comments on users table
the patch to test removal, also subject should use formatter and widget, will file new issue
Comment #15
andypostComment #16
andypostFiled #2292821: Use widget for comment subject field
Comment #18
andypostre-roll after #2292815: Remove join comments on users table
Comment #20
andypostComment #22
andypostComment #24
andypostStart to fix views integration
Comment #26
andypostfix some tests
PS: pushed to https://github.com/andypost/drupal/tree/1498662-comment-subject
Comment #28
andypostfix forum and tracker
Comment #30
andypostExtended and fixed
Drupal\content_translation\Tests\ContentTranslationSettingsTest
Now subject field is translatable by default so content translation always set it checked.
Comment #32
andypostFix
Drupal\entity_reference\Tests\EntityReferenceSelectionAccessTest
Comment #34
andypostFix default views
Comment #35
andypostfinally green, waiting reviews
Comment #36
larowlanGreat work only some minor observations.
Could this be rewritten as
Is this the only thing that becomes translatable?
This comment could use a tidy up - suggested
'If no conditions join against the comment data table, it should be joined manually to allow node access processing'.
Whoops, this should have been gone by now, seeing as though we have had comment-bundles for eons.
%s/none/non ?
Comment #37
Gábor HojtsyGreat job @andypost, @larowlan, let's fix the remaining items and get this in :)
Comment #38
amitgoyal CreditAttribution: amitgoyal commentedPatch in #34 no longer applies. Please review revised patch along with fixes in #36.
I am not sure about #36-2.
Comment #40
andypostRe: #34
1) changed
2) Looking on Node seems others needs too - status, created, changed, uid (and parts) also suppose "thread" ML, this will allow to have thread per entity translation.
3) fixed
4) yep, magically this does not affect us before...
5) Suppose Untranslatability is proper term.
Comment #41
andypostSuppose thrading per translation needs more tests and would be fixed after #2156089: Remove comment_get_thread() in favour of method on CommentStorage
Comment #43
andypostFix broken test
Most of properties are multilingual, so only "thread" needs to check that comments are attached to translation
Comment #44
Gábor Hojtsy@larowlan: looks good? :)
Comment #45
larowlanLooks good, great job @andypost
Comment #47
larowlanStraight re-roll
Comment #49
larowlanFixed re-roll
Comment #50
larowlanjust a re-roll, back to rtbc if green
Comment #51
andypostDiscussed with Gabor in #d8mi meeting - thread should not be translatable, parent (thread) can't be changed while translating
Also fixes merge conflict
Comment #52
larowlanStill RTBC
As this makes data changes, really need this in before beta
Comment #53
plachI have to admit I don't understand every single line of this patch, but overall it looks good to me :)
Just a couple of remarks:
Do we really want to allow people to store a different host per language? ;)
In these cases hard-coding the check against the default language won't always work as we might have different statuses per-language. We should open a follow-up to review these queries with a multilingual forum.
Comment #54
alexpottPretty sure that a change record is required here.
Comment #55
andypostCR added https://www.drupal.org/node/2301459 and re-roll after #697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
Filed follow-up #2301465: Make comment threading per entity translation to address #53.2
@plach #53.1 yes, hostname is a part of author information so should be able to store translator's data
Also https://www.drupal.org/node/1722906 needs to link the issue
Comment #56
andypostfixed summary
Comment #57
alexpottCommitted 837d726 and pushed to 8.x. Thanks!
Comment #59
Gábor HojtsyWoot, amazing!
Comment #60
andypost