Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#144 | drupal8.comment-module.2079093-144.patch | 449.11 KB | andypost |
#143 | interdiff.txt | 2.75 KB | dixon_ |
#140 | interdiff.txt | 2.75 KB | andypost |
#140 | drupal8.comment-module.2079093-140.patch | 449.02 KB | andypost |
#139 | interdiff.txt | 7.79 KB | andypost |
Comments
Comment #2
larowlanComment #4
larowlanFor #2
Comment #5
larowlanComment #6
larowlanComment #8
larowlanrinse. repeat.
Comment #10
larowlanOk, this should look a lot better, getting some green locally now.
Switched bundle to {entity_type}__{field_name} aka field_id with '.' changed to '__', '.' conflicts with CMI
Added computed property field_name
Comment #12
larowlanFixes CommentFieldTest (big patch coming later)
Comment #13
larowlanFixes CommentInterfaceTest
Comment #14
larowlanFixes CommentLanguageTest
Comment #15
larowlanFixes CommentNodeChangesTest and CommentNodeAccessTest
Comment #16
larowlanSeeing where total fails is at, more tomorrow
Comment #18
yched CreditAttribution: yched commented@andypost pinged me on tweeter
Sorry guys for making your life more difficult (again) on this patch :-/.
Is there anything specific on which you need feedback ? AFAICT, using (a variant of) the $field->id as bundle, instead of the field name previously, makes total sense.
Comment #19
andypost@yched Thanx for attention!
The main issue here that
getFieldById()
uses uuid as argument so logic goes complex because$field->id()
could not be used at all.The second one is that field API can't attach fields and ui on composite bundles (entity_type [delimiter] field_name) and no way to use bundle with a dot 'node.comment'
Comment entity needs bundle to attach fields. Previously we use field_name because it was unique now
field_id
looks promising because of the uniqueness but it have a dot '.' as splitter for entity and field.So field_ui can't work with it, probably because some cmi related limits (suppose @larowlan will provide details) some of them was solved in #1831774: Config import assumes that 'config_prefix' contains one dot only also using field_id caused a lot of
list($type, $field) = explode('.', $field_id)
that makes code harder to readAs comment field could be attached to any entity we need to know entity type of the commented entity and field name via the thread attached. Also they are needed to provide proper views integration. Also currently we have no ability to load a field by it's ID.
Comment #20
larowlanMerges HEAD and fixes forum and some upgrade issues
Comment #21
larowlanFixes comment translation ui test
Comment #22
larowlanFixes ForumViews and ForumBlocks tests
Comment #23
larowlanFixes COmmentUninstallation and ContentTranslationSettings tests
Comment #25
larowlanFixes RDF, Search and other tests
Comment #27
larowlanFixes upgrade and ER fails
Comment #29
larowlan#27: comment-field.reroll.27.patch queued for re-testing.
Comment #30
larowlanComment #31
andypostLet's continue to roll statistics patches here, it's too hard to scroll the helper issue
Comment #32
yched CreditAttribution: yched commented@andypost:
I don't have much context, but I don't get why you try to use getFieldById(). getField($entity_type, $field_name) sounds more appropriate ?
But i though you were using "[entity_type]_[field_name]" for the bundle ? I don't see why field / field_ui would stumble on that, it's just a string ?
You should probably use '__' (double underscore) as a separator if you want to avoid ambiguity / clashes (case of entity type and / or field names containing '_' --> where is the separator ?)
Comment #33
andypostbecause it requires a bundle to attach it's tabs to manage fields and etc
So for now we proceed with '__' as splitter
Comment #34
yched CreditAttribution: yched commentedYes, and it should work if you use a bundle name that's [a-z0-9_], which is what you are doing, right ?
So, it doesn't seem like there is a problem ?
Bundle names should be [a-z0-9_], really. Many things outside Field UI will break if they aren't.
Comment #35
andypostTest the merge of HEAD, current code pushed to github to not bring a mess to sandbox, I'll try to fix thing and then push to sandbox
Comment #37
larowlanComment statistics are properties, fixed hidden not shown on default widget
Comment #39
andypostthis expensive calls to service should live inside if()
Comment #40
larowlanFixes #39 and some of the fails (hopefully all)
Comment #41
larowlanApplies default values correctly
Comment #42
andypostThe proper method to use
massageFormValues
PS: not pushed, just to test the approach
Comment #44
andypostChasing HEAD
Comment #45
andypostStupid copy/paste
Comment #46
andypostLet's try clean removal
Comment #47
andypostFix the rest and extend tests for admin screens, check for hidden option should be tested too
PS: interdiff needs a bit of work, so not pushed
Comment #48
andypostmerged comment_reply changes and breadcrumbs, fixed tests
@larowlan comment_count have issues so hidden and default for new is always open
Comment #49
andypostAnd removal remains
Comment #50
andypostre-roll
Comment #51
andypostanother round of tremendous merge
Comment #52
andypostFix user and disabled tests
Comment #54
andypostFix for the rest
Comment #55
andypostproper merge
Comment #56
larowlanRe-roll after #1983554: Remove BC-mode from EntityNG , #2053489: Standardize on \Drupal throughout core, #2051097: Change "pattern" to "path" in *.routing.yml files
Also refactors applyDefaultValue logic as follows:
Tipping fails as BCDecorator is gone
Comment #58
larowlanMissed some pattern|path changes
Comment #60
larowlanThis should move in the right direction
Comment #62
larowlanWidget changes too
Comment #64
jibranGreat job @larowlan and @andypost. Here is a less technical review of the patch. Feel free to ignore or kick anything to follow up. Will review tests an onward later.
Why is this 'deny' and not 'DENY'?
Is this pending or will be fixed in followup? Perhaps an issue id if it is a followup.
I think we can remove this function. @dawehner and I removed the usage of this function in comment_reply conversion. If we can't get rid of this then maybe move it to manager. We can do it in followup as well.
ahhh
We should add @todo here or change it to entity.view.
entity.manager now.
\Drupal\Component\Utility\Xss::filter() now
I don't get the reason why we need to do this if the route is comment.reply then we'll get these param any way.
This is out of scope but can we please at least use \Drupal instead of wrapper functions.
Please convert to multi line.
hmmm user_acess
currentUser service.
Injection.
Drupal 9 and issue id.
Injection.
Comment #65
larowlanRemaining fails are because of new applyDefaultValues hunk at #56- can someone from the Entity team shed some light on the right way to do this?
Comment #66
amateescu CreditAttribution: amateescu commentedAnyone minds if I try a patch here not related to the comment issue? I hope not, `cause I'm posting it anyway :)
Comment #68
larowlanSome fixes for #64 and hopefully more passes.
Comment #70
larowlanMore fixeration
Comment #72
larowlanMore fixeratation - default values seem to be working now!
Comment #73
larowlanWhat's your favourite colour baby....Green
Comment #74
larowlandisabled modules went in
Comment #76
larowlanshould fix those 4 fails
Comment #77
dsnopek@larowlan: Thanks so much for your work on this patch! I know it's been a rough road having to spend so much time re-rolling this massive patch. But thank you for carrying this burden and I really hope this gets committed soon!
Comment #78
andypostand another re-roll
Comment #79
andypostComment #81
andypostshould fix
Comment #82
andypostComment #83
andypostfixed broken test
Comment #84
larowlan@dsnopek thanks for the words of encouragement although many others have worked on this. @andypost has re-rolled this as many, if not more, times than me. @dawehner did most of the views integration and @fubhy did the first cut of the new admin pages. And don't forget the reviewers @alexpott, @xjm and @berdir have poured over this large patch several times and @tsvensson has done a number of UX reviews.
We're close and the end is in sight. Hoping to get this to an RTBC state next week, putting aside one night next week for a full-review and tidy-up and some test-fixes. Wouldn't it be sweet if it was live-committed during Drupalcon? A 12 month effort nearing the end.
Comment #85
larowlanchases HEAD
Comment #87
larowlanOk so this fixes a few fails, and a few of the issues identified in reviews in #1907960: Helper issue for "Comment field" (see interdiff).
But we're stuck on 4 fails that relate to render caching and access.
We view a comment-thread as user x who has 'access comments' permission and the comment-output (from the formatter) is cached.
Then we view the same thread as user y who doesn't have the permission but the cached output is shown.
Can we use #access callbacks on the cached render array? I can use #access => $this->currentUser->hasPermission('access comments') but I assume that is cached as TRUE anyway, evaluated when the first user views it. So it boils down to can we have #access cached in a render array as a callback? If not we need to be able to.
Have sent question to @berdir and @andypost on irc.
Comment #88
Wim LeersRender caching is per-role (by default at least, as introduced in #1605290: Enable entity render caching with cache tag support), so I don't see how that could happen? Am I missing something?
Comment #89
larowlanOk, so could be I'm missing something else, thanks Wim, that gives me an indicator that this should be fixable.
Comment #90
yched CreditAttribution: yched commentedNot sure what the need / use case is, but overriding applyDefaultValue() looks a bit weird. Typically field types will want to override getDefaultValue() instead - override "what the default value should be" rather than "how/when it gets assigned in an entity".
Comment #92
amateescu CreditAttribution: amateescu commented@larowlan, we discussed this topic in Prague today and unfortunately it's a thing that entity render caching doesn't support *yet* (but it needs to) and we outlined a plan for it. My suggestion is to comment out that test for now, but definitely keep it in the patch because it will be very useful to have once we get the other pieces in place.
Comment #93
andypost@yched suggested in IRC:
but this does not works because EntityNG::getTranslatedField() does not apply default values when no data saved for entity.
The scenario:
1) visit
admin/config/people/accounts/fields
and add some Comments field2) do not save field and instance settings (so no defaults are saved to field and instance)
3) visit
user/1/edit
no default value for comment field selected for user!@larowlan The logic for widget should be following:
1) try to get status from
$items
(actual field values if any)2) if no data, fallback to field defaults (but currently the
Comment #94
andypostSo here's a fixes for default values:
Comment #95
larowlanComment #96
yched CreditAttribution: yched commentedActually, if the goal is to set an initial value for the "default_value" property in newly created FieldInstance definitions, then hook_field_instance_create() might cleaner place than messing with either getDefaultValue() or applyDefaultValue() ?
Comment #97
andypost@yched this is a half of the problem, the second part is absense of data when field added to existing entities - that's #93-94 mostly about.
So nice solution would be to use
hook_field_instance_create()
to provide defaults for field instance and once there's no data in entity have a fallback to instance defaults!Comment #98
andypost@larowlan done a lot of clean-up/comments fixes let's test
Comment #99
dixon_One of my main goals now during DrupalCon is to get back into this issue again and review and test it thoroughly. I'll also try hard to lobby for this getting committed with some follow-ups, as soon as possible. It's a big patch and very heavy to keep up with.
Comment #101
dixon_I only got started reviewing, but here's a start with some nit-picking:
This comment probably doesn't make sense any more.
This should be
\Drupal::
.Should also be
\Drupal::
.The "eg" shortening is missing a trailing dot, should be "eg.". And there's a space within the second link tag there.
Let's remove this then :)
This is a view now. Is there anything we need to change?
Let's post a follow-up for this and remove the comment.
Comment #102
dixon_Comment #103
andypost@dixon_ thanx for review, fixed 4bd944c
Also fixed Render controller to use
\Drupal::currentUser()
without injection because of #2076411: Remove the request scope from the current user serviceComment #105
larowlanSo feast on this interdiff from yesterday's work.
And this should resolve more test fails, should be back to the four related to entity render cache - see #92
Comment #107
larowlanWrong window blues.
Should be down to 5 fails (one token one that passes locally and four for render cache at #92 issue)
Comment #109
larowlandoh wrong file
Comment #111
larowlan#109: comment-field-reroll-9000.patch queued for re-testing.
Comment #113
larowlanFixes comment token tests and a bucketload™ of other stuff from the review to-dos
So this means we're down to the cleanup of CommentUserTest and refactoring it to use entity_test_render instead of user (assuming thats still required).
And the render-cache fails as per #92 but thats an issue in HEAD (@amateescu said comment those asserts out).
So keep those reviews coming - lets get this in during Drupalcon.
Comment #114
larowlangroan
Comment #115
yched CreditAttribution: yched commentedre @andypost #97
So, two things here:
- making sure that newly created field instances get a 'default_value' property: this is what hook_field_instance_create() is for.
- handling the case of "when creating a new comment field, preexisting entities do not have a value" : CommentWidget::formElement() is not the place to address that. What you want is "a comment field can never be seen as 'empty', it should always be seen as at least 'has one value, COMMENT_OPEN'." Discussed this with @Berdir, this should be done by overriding CommentField::__get() so that it returns a value even if there is none.
Comment #116
BerdirYes, the unfortunate thing about that is that you will also have to override get(), as both could be called.
Comment #117
yched CreditAttribution: yched commented@Berdir: would it be easier if we override __set() instead ?
Comment #118
andypostFinally pushed http://goo.gl/3sO7Jp
Comment #119
dixon_I'm running a bit back and forth on the conference here, so I'm sorry for breaking up the review so much. But here's another chunk of nit-picking:
I don't think you intended to add this to the patch? :)
Disabled modules are in, I believe. Should we remove this then?
Remove this as well then?
Let's remove this @todo since there's an issue for this already.
I've never seen this doc/comment pattern before. Maybe it's enough with just
Implements hook_form_FORM_ID_alter().
. I think that's descriptive enough.Same here and on more places, seems a bit verbose IMO.
Let's remove this @todo since there's an issue already.
I think the
$comment_entity
variable name is a bit confusing here. One could easily think it's the comment entity itself.Elsewhere in the code I've seen
$commented_entity
which I think is a lot better.To be consistent with the previous point we should use
$commented_entity
here instead? One could easily think this is an actual comment entity.Comment #120
dixon_Let's test #118
Comment #122
dixon_Here's more reviews based on the latest patch:
Let's remove this since we have an issue for this already.
Should use
\Drupal::
.\Drupal::
on two places here as well.Should we document this in the 'help' key to make this obvious to the end-user?
Grammar error. It should be "We cannot create a relationship"
Should not have capital letter 'E' here.
Should not have capital letter 'F' here.
Should be
\Drupal::
on two places here.No, it doesn't ;)
Not this either ;)
Let's do {@inheritdoc} here and all other places on these classes.
Let's remove this since issue already exists.
Comment #123
andypostFixed broken tests - added @todo to properly implement render cache per-user
CommentUserTest
@dixon is there an issue for that?
Addressed review #119 & #122
Finally fixed default values as @berdir suggested
Comment #124
yched CreditAttribution: yched commentedI think it's the __get() of the CommentField class that should be overriden, not the one of CommentItem.
When the *field* is empty on an existing entity, we want it to say "no, pretend I'm not empty, I have one FieldItem, here it is".
Comment #125
andypostAdded lost title callback that still needed to display breadcrumbs properly.
Simplified code for admin controller.
@yched suppose we need
__get()
forCommentItem
because the *field should never be empty* asisEmpty()
method tells.Comment #126
yched CreditAttribution: yched commented"field should never be empty" is something that deals with the Field (list of FieldItem objects) level, not each separate FieldItem level.
--> CommentField::__get() is the one you want to work on.
Comment #127
andypost@yched yes, that's why we do not implement
CommentField
for 'status' property|item of theCommentItem
(Field)While all fields are 'list of items' and no way to implement single value field #1869574: Support single valued Entity fields we should hide UI to change cardinality and using
$items->status
to work with first item only. I'd like to point #1751210: Convert URL alias form element into a field and field widget that got this trouble tooEDIT also probably related #1988492: Avoid having empty field items by default
Comment #128
larowlanRefactored tests to use entity_test_render.
Some of the generic changes filed as #2097981: Expand EntityTestRender annotation to add more useful functionality for testing fields
GoodGreen feeling, won't you stay with me, just a little longer...Updating issue summary on main issue.
Comment #129
larowlanDid an @todo pass to make sure we had em all.
Added one more followup.
Fixed mucked up checkbox order.
Comment #130
larowlanThis is going to fail because #2097981: Expand EntityTestRender annotation to add more useful functionality for testing fields did (which is included in this patch).
Comment #131
dixon_Dumping some notes and quick nit-picks here before hitting bed.
I believe the code is in very good shape, I'm mostly running into docs and todos that needs cleanup.
Should use capital letter 'C'.
"Make sure the field..." sounds better, IMO.
Let's remove this @todo since it has an issue.
Should have leading capital letter 'M'.
The comment line break seems a bit early here.
This @todo has an issue, so lets remove the comment.
This @todo also has an issue. Let's remove it.
Comment #133
larowlanFixes fails
Fixes #131
Comment #135
larowlan#133: comment-mcfield.patch queued for re-testing.
Comment #136
andypostOld 'title callback' #125 should be removed because we use path-based breadcrumbs https://drupal.org/node/2098323
Comment #137
dixon_Here's a continuation of the review. I'm gonna review the last bits tomorrow at the sprint. Then, we also need to tackle the render cache thingie...
Are we sure this is the best way to do this? Wouldn't it be better to set it to hidden by default for these view modes?
It might be out of scope. But maybe we can remove most of the logic in
comment_node_update_index()
in favor of doing it properly through the formatter?That said, I'm ok for leaving it like it is for now. We can treat that as an optimization later, since it would only move around some code and not break any behavior really.
This comment is breaking line a bit too early.
I'm not sure where we are "loading" any table information. And we're not really joining per either (although subselect could definitely be seen as a type of join of course).
Maybe it's more straight forward to just say "Use the table definition to correctly add this user id condition."
This should be
$this->getEntity($values)
.Uh, what's up with this? :)
Missing trailing sentence dot.
Indentation error.
Should not have capital 'D' here.
Let's remove since we have an issue for it.
Comment #138
andypostfixed all from #137 and a bit of clean-up, can't push now so just interdiff and patch
We can't use disabled entity display for render now ;( so that's the only way now
Second line is too long and contains controller and method to test
Comment #139
andypostUncomment tests, also the whole interdiff also addresses #136
Comment #140
andypostFiled #2099105: Clean-up render cache when permission changes
Also added fix for render cache when instance settings changed
Comment #142
larowlan#140: drupal8.comment-module.2079093-140.patch queued for re-testing.
Comment #143
dixon_Ok, made it through the whole patch finally. Here are my, for now, final remarks. I took the freedom to push these minor changes to the sandbox.
I'll update the main issue in a moment when #140 comes back green.
Should be
\Drupal::
on both places.Should be
\Drupal::
on two places here as well.Same here.
Comment #144
andypostClean merge HEAD after #1953528: Store 'per-bundle view mode settings' in CMI, get rid of field_bundle_settings() & its variable
Comment #146
andypostNo more needed!