Updated: Comment #0
See Issue summary for #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title)
This issue is about the following base fields of the Node entity type (basically everything except the title):
- author (
uid
) - publication date (
created
) - language (
langcode
) - published (
status
) - stick at top of lists (
sticky
) - revision log message (
log
)
This includes the addition of new widgets / formatters:
- author (
uid
):AuthorFormatter
(generic, forEntityOwnerInterface
entities) +RouteBasedAutocompleteWidget
(generic, to not depend onentity_reference.module
) +AuthorAutocompleteWidget
(extends the previous one with author-specific UX) - publication date (
created
):TimestampFormatter
(generic) +TimestampWidget
(generic) - published (
status
) & stick at top of lists (sticky
):BooleanWidget
(generic) - revision log message (
log
)
Only the language (langcode
) field (and its corresponding LanguageWidget
) will not be done in this issue, for that we have #2230637: Create a Language field widget and the related formatter.
And for datetime.module
's "enhancing override" to switch the default text field widget to a fancy datetime widget now has a DateTimeTimestampWidget
that can be used for any TimestampItem
— this also cleans up datetime.module
.
Also removes node_field_extra_fields()
! :)
And last but not least, this finally makes the node author and date in-place editable! Or well… almost. In-place editing the author fails, for reasons that swentel and I don't yet understand. Hopefully yched, fago or amateescu can shed some light on that.
Remaining tasks
- Fix test failures.
User interface changes
None.
API changes
None. The base fields above are rendered using widgets and formatters, and are no longer exposed as an "extra field".
Comment | File | Size | Author |
---|---|---|---|
#205 | 2226493-205-interdiff.txt | 589 bytes | Berdir |
#205 | 2226493-205.patch | 66.06 KB | Berdir |
#201 | interdiff.txt | 2.51 KB | joelpittet |
#201 | 2226493-200.patch | 66.06 KB | joelpittet |
#193 | 2226493-193-interdiff.txt | 2.39 KB | Berdir |
Comments
Comment #1
Wim LeersComment #2
Wim LeersComment #4
Wim LeersLots of conflicts in rebasing. I hope I got it all right.
Comment #6
Wim LeersOops.
Comment #8
Wim LeersThis is turning into a real clusterfuck. I fixed one annotation incorrectly.
I still can't install it locally, but I don't get a useful error, so… testbot to the rescue!
Comment #9
BerdirWould be useful to have an issue or something here...
This is awesome :)
This should be the default for base fields.
Interesting, should the revision log maybe be a string_long? Haven't checked.
Just copied I guess, but is that even valid english? :)
this is called for a specific bundle, you should just use that and not loop over all of them.
Should also make it configurable?
Now that it's a widget and the field attach form stuff landed, this might work based on the validation API now?
Same here.
Should also not be needed anymore if the widget is working correctly.
Would be nice if this could be a part of the widget logic too, but might be node specific, not sure.
Comment #11
BerdirAlso #2226063: Merge ListBooleanItem from options module into BooleanItem is merging the boolean field and adding widget/formatter for it too.
Comment #12
mr.baileysAssigning to myself to try and resolve the test failures during the sprint.
Comment #13
mr.baileysI was able to fix some of the failing tests, but then ran out of time. Attached is the result of my work in case anyone wants to pick up were I left. If not I might be able to continue later this week.
Settings to Needs Review to get a new test report.
Comment #15
mr.baileysWorking on this together with Wim Leers.
Comment #16
mr.baileyswork-in-progress...
Comment #18
mr.baileysAnother test failure fixed, this time with *a lot* of help from Wim Leers & fago.
Comment #20
andypostOverall great, the main issue here that Core and modules responsibility mixed.
Boolean field has own issue #2226063: Merge ListBooleanItem from options module into BooleanItem
Entity reference formatter could move to Core 2 default ER module (label and string) but autocomplete widget require route, but Core can't use routes.
Lack of the route support leads to really bad DX
Maybe better to use here a ER label formatter by moving it to Core\Field\...?
Any reason in this variable and assignment?
why not add "changed" here?
Related issue #2226063: Merge ListBooleanItem from options module into BooleanItem
Trying to de-duplicate boolean fields already... Maybe join efforts?
DX--
So each time I add user_atocomplete I need to remember the route?
No more after #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition
defined in language module, Core should not depend on it!
Comment #21
Wim LeersWe should also be able to remove this — at last :)
Comment #22
Wim LeersAnd one more : we should be able to remove this hack too:
#2114707: Allow per-bundle overrides of field definitions landed, so we can do this in
Node::bundleFieldDefinitions()
:)Comment #23
mr.baileysStill need to fix the remaining test failures and adress the remarks from #20.
Comment #25
Wim LeersAfter a discussion with plach, it was decided that it's not feasible to create a
LanguageWidget
in this issue, because there is too much code to untangle, which would significantly expand the scope of this issue.To handle that, I've opened #2230637: Create a Language field widget and the related formatter.
The next reroll of this patch should no longer include
LanguageWidget
.Comment #26
scor CreditAttribution: scor commented+1. If we have author and created date as generic field formatters, we could remove some the custom code from rdf.module that currently deals with adding RDFa markup to author and date in nodes and comments output (aka submitted by). I believe this issue would be easily solved too: #2047095: Remove $submitted from node templates.
Comment #27
BerdirRe-roll and reverted the langcode changes.
Comment #29
BerdirFixed some tests, but the fixes are pretty ugly. Not sure why the author widget is even used for EntityTest although the fix makes sense, but the translation controllers are ugh.
Comment #31
mr.baileysThis should address a lot of the remaining test failures.
Comment #33
mr.baileysI think I got it down to a single failing test.
Comment #35
BerdirAsked @scor about how to properly deal with this in rdf.module, it's something that's existing in HEAD but is not visible as the created field doesn't have a view display component so the field mapping there is ignored.
Two things that I can thing of:
- Add a different callback function that is able to deal with an array('value' => 345365) structure for those fields
- Allow to define mappings for field properties instead of fields, so that you can define created.value instead of created.
@scor also had a different idea I think, assigning to him.
Comment #36
scor CreditAttribution: scor commentedreroll.
Comment #37
scor CreditAttribution: scor commentedI decided to take the approach to use a different callback function that is able to deal with an array as value.
Comment #39
andypostnot sure this change is needed,
get(created)->value
should return the sameComment #40
scor CreditAttribution: scor commented@andypost nope,
$variables['node']->getCreatedTime()
returns a UNIX timestamp string and$variables['node']->get('created')[0]->getValue()
returns an array('value' => {timestamp}).Comment #41
Wim LeersI cannot reproduce the fails in
Drupal\edit\Tests\EditLoadingTest
locally.Fixed the
Drupal\node\Tests\NodeTranslationUITest
failures.Comment #43
Wim LeersI still can't reproduce those last 3 failures (and 2 related exceptions) in
EditLoadingTest
. I wonder if it's because I'm on PHP 5.5…Comment #44
Wim LeersThis is also blocking the critical #2091401: Add hook_help for Quick Edit module.
Comment #45
aspilicious CreditAttribution: aspilicious commentedI can be wrong but didn't we move the settings out of annotations? Or is that issue not in yet...
Comment #46
yched CreditAttribution: yched commentedComment #47
mr.baileysI have some time today to re-roll this patch and attempt to fix the remaining test failures.
Comment #48
mr.baileysRe-rolled to keep up with HEAD.
You're correct, somewhere between patches, #2136197: Move field/instance/widget/formatter settings out of annotation / plugin definition got committed. I updated this patch so settings are moved out of the annotations and into
defaultSettings().
QuickEditIntegrationLoadingTest was now failing with array to string conversion warnings in
theme_field__node__*
, fixed by wrapping$variables['attributes']
innew Attribute()
.PageCacheTagsIntegrationTest was failing again. I have removed the "user_view:1" from the list of expected tags (this was added earlier in this thread, but since the 'submitted'-text is still hard-wired rather than rendered, this tag is not present). Not really sure about this one though, would be great to get input from someone who knows more about cache tags than I do.
Comment #50
webchickAs much as I would still love to see this happen, since it's very silly as an end user to only be able to in-place edit some things but not others on a node, the core maintainers discussed this today on a call going through all of the outstanding criticals, and we are very hard-pressed to make this a release blocker atm. It doesn't represent a regression from Drupal 7, and while this issue was originally marked critical as a blocker to hook_help() (another critical), but that's a false dependency; the docs can still be written and then just amended later when/if this makes it into core. (Hopefully, "when." :))
Comment #51
Wim LeersRerolled.
Now has Twig templates for the
uid
andcreated
fields on nodes, just liketitle
has since #2216437: Entity labels are not in-place editable on "full entity page" (prime example: node title).The comment settings are now misplaced; they no longer end up having vertical tabs, and hence don't end up in Seven's "secondary form region". They work just fine, but are placed incorrectly. Currently debugging that, but in the mean time testbot can have a look at this :)
Comment #53
Wim LeersThis is correct.
Since #2099131: Use #pre_render pattern for entity render caching, it's now the responsibility of formatters to declare cache tags, which explains this difference over time.
(Technically, the
user_view:1
tag should appear, but it cannot, becauseuser_view($node->getOwner(), 'compact')
is being invoked in the theme layer (intemplate_preprocess_node()
), which is the wrong place to be rendering entities — it makes it impossible to bubble up cache tags. Fixing that is definitely out of scope for this issue.)I was finally able to reproduce the failure in Quick Edit (the one that was responsible for the last few test failures back when this patch was almost ready — see #43), and fixed that!
We should now be at zero failures.
That leaves us with two todos:
Comment #55
Wim LeersShould be green.
Comment #56
Berdirunnecessary and wrong (indentation) left-over...
This override stuff is really crazy, but I don't know how else to clean it up...
This looks close.
You might notice that we're adding a lot more code than it removes, but most of it is in more or less re-usable widgets that other entity forms will be able to use as well.
Comment #57
Wim LeersThanks for the review!
With that, I know of no more remaining issues! Finally! :)
Comment #58
Wim LeersRerolled now that the PSR-4 patch has landed.
Comment #59
mr.baileysComment #60
moshe weitzman CreditAttribution: moshe weitzman commentedLooks like all outstanding issues have been addressed.
Comment #62
Wim LeersStraight reroll; #2276183: Date intl support is broken, remove it broke this in a minor way.
Back to RTBC per #60.
Comment #64
Wim Leers#62 included one remaining call to
datetime_default_format_type()
, which was removed in #2276183: Date intl support is broken, remove it. Now fixed.Comment #66
Wim LeersForgot to pull for #64. Straight reroll.
Comment #68
Wim LeersSigh. Deep sigh.
/core/modules/datetime/lib/Drupal/datetime/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
->
/core/modules/datetime/src/Plugin/Field/FieldWidget/DateTimeTimestampWidget.php
(Sorry for all the noise.)
This fixed most of the fails, but there was still a fail in
EntityTranslationFormTest.php
, which I also fixed.Comment #69
Wim LeersBack to RTBC per #60.
Comment #71
Wim Leers#2247211: Autocomplete widget doesn't work on entity labels without a " (entity_id)" suffix broke two hunks of this patch. Straight reroll.
I think this patch is sufficiently important & large to be marked to avoid commit conflicts.
Comment #72
andypostI still think that one needs follow-up to add tests for newly introduced widgets and formatters
Comment #73
tstoecklerOverall the patch looks great, lots of stuff to love. Thanks for all the work on this, impressive!
I have a bunch of remarks. Most of them are related to injecting things and allowing for better unit testing. You can safely ignore those, if you don't want to bother with this now, we can also handle that in a follow-up. There are a few items, that I am marking this needs work for, though. Will comment in the next comment which items those are (because I can't preview my comment...).
theme_username() is in User module, so
AuthorFormatter
should be as well.Let's not use the deprecated
format_date()
but instead let's use\Drupal::service('date')->format()
. Instead of calling that directly, though, please use aprotected function date()
to allow for unit testing.Again, this widget should belong to User module.
Also it would be great to put
\Drupal::config()
into aprotected function config()
. Similarly, instead of usinguser_load_by_name()
you could add aprotected entityManager()
or eventuserStorage()
and useloadByProperties()
or alternatively aentityQuery()
to perform the entity query manually.This should include a check for
$entity instanceof EntityOwnerInterface
and throw an exception if it isFALSE
.As the
route_name
setting has to be set from code, I think this shouldn't be available from the UI, right? I do not see ano_ui
here, though. (And I think that's not even possible for widgets ATM.)I cannot even remotely parse what this line is doing. Please split it into multiple lines.
I do not understand the discrepancy here.
You used
\Drupal::formBuilder()
above, so can we do it here, as well? Again,protected function formBuilder()
would be preferred.YAY!!! :-D
Similar to above please use a
protected function entityManager()
or alternativelydateFormatStorage()
.Oh wait, we have
DateFormat::load()
now. So while the former would still be preferred the latter would at least be an improvement.These are tests, so you don't have to go all injection-y here, but at least
User::create()
would be nice.Again,
EntityTest::create()
would be preferred.Yay!
Is the
TRUE
required or just a performance optimization. Although it sort of makes sense, it seems like that is something that Contrib could get wrong easily. Or would contrib not have to do anything similar?Until #2236903: setSettings() on field definitions can unintentionally clear default values gets in using
setSetting()
multiple times instead actually is mandatory oversetSettings()
.Oh, I just love all the removals in this class. Yay!
At least
User::load()
would be preferred. AFAIK translation handlers actually have acreateInstance()
so we could properly inject the entity manager or even the user storage here.Please use
Format::string()
.The
[0]
should be handled automatically, I think. Otherwise, please usefirst()
.I think this is fine for now, but this is problematic as
string_long
does not support text formats. Not sure what would happen with astring_long
field type if I settext_processing
toTRUE
. Can we at least add a @todo here to add a dedicated formatter?Comment #74
tstoecklerSo I would ask that 1,3,5,6,7,14,15,19,20 be adressed in some form, if possible. The rest would be nice as well, but since this has been so much work already please feel free to ignore and I will open a follow-up. Thanks!
Comment #75
BerdirJust a re-roll for now.
Comment #77
BerdirUps, missed a few merge conflicts.
Comment #79
BerdirFixing those test fails.
Comment #80
alansaviolobo CreditAttribution: alansaviolobo commentedComment #81
Berdir1. Moved.
3. Moved. Did not change the other things yet.
4. This is a widget, so it should directly use the field item, not go through the entity, not sure why it did that, we'll find out if there are test fails I guess. Not sure about the name of those two classes, maybe it should be User instead of Author, as it's not limited to be used for the Author/Owner?
5. It being a setting has nothing to do with UI, there would only be a UI if we'd implement a settings form, which we don't. So it's a setting that has to be set in code.
6. Rewritten that, also used REQUEST_TIME as an example, no need to depend on getCreatedTime(), especially as it's just a example value *and* it would actually be the same as the $default_value in this case.
7. Neither do I, but i guess that's existing code, not sure about touching that here. They're using completely different form types, so I'd expect that there are differences.
12. EntityTest::create() actually doesn't work because of the changes that @fago made me do :( My mistake, but I blame him :p (We broke the real use case of having (multiple) subclasses by trying to implement the theoretical use case of the same class being used for multiple entity types).
14. Pretty sure this is a performance optimization only, to avoid further cache collects. @Wim: Can we use that as a quick fix/workaround for the insanely slow testing table rendering, and make sure that this is passed to the sub-theme functions for theme_table() ? I see that table is now a twig template, not sure how that affects things exactly.
17. translation handlers are nothing but a bunch of hacks put in a class, that's not going to make it prettier ;)
18. I think you meant String::format() :p
19. It's only handled automatically if you access a single property, getValue() gives you a list of items with their values otherwise. Still consider that code crazy ugly, we shouldn't rely on that structure, but I made it a ->first()->toArray() for now at least.
20. Not sure that is necessary, I don't think anything would actually break and it's not an existing, setting of that field type, so you're not allowed to set it anyway. And if we really wanted to, we could check if the item has a format property additionally to the setting.
Open:
2., parts of 3., 8, 10, 11, 12, 17, 20.
Comment #83
BerdirInteresting test fail ;)
I think we need to introduce no_ui for widgets too, because RouteBasedAutocompleteWidget (should this even be a widget, it doesn't seem to be usable without a value massing/validation method?) really shouldn't show up in the UI, and author also not, or we'd need a supports($field) method on the widget, because it only works for user references.
Comment #84
yched CreditAttribution: yched commentedYes, "author widget" intrigued me on a very cursory read. What's the need for a dedicated, specific widget ?
The problem with making those "internal widgets only" is that the base fields that use them cannot have their "form display settings" exposed in the UI either (since their initial widget is not available in the UI).
Letting people switch-in powerful entity ref widgets from contrib (like the very promising https://www.drupal.org/project/entity_browser) was one of the big wins about moving "author" to a field + widget...
Comment #85
BerdirThe entity reference widgets are provided by the module, using it from node would require to make entityreference practically a required module. We can't just move it to Drupal\Core as it depends on the whole selection plugin stuff.
I think @Wim tried to avoid making any functional changes, and this would be one example that would be one, the empty behavior is different (defaults to anonymous user if you leave it empty, the entity reference widgets would have to be required and explicitly selected), the autocomplete selection behavior would be different (it mostly relies on the ID, when given, the current one just matches the username). And at least when this issue started, the exact-single-match mode without ID in entityreference was broken (and still is in 7.x :(, for a long time now...).
Also, it's quite likely that the entity reference widgets only worked for configurable fields when we started this issue, we fixed that not too long ago.
All that said, I agree that we should seriously consider to just rely on entityreference, use that widget and move on, as we'll get a lot of nice stuff for free. I'll try to switch, then we'll see what happens with the tests.
Comment #86
BerdirLet's see what happens. At least the selecting the anonymous user seems to be broken, autocomplete displays it but then validation fails.
Comment #88
BerdirOk, fixed those fails (could not reproduce quickedit after fixing the others), we do have a UX regression on the validation message :(
Also, did not yet switch the formatter.
Comment #89
Berdirnow also using the entity reference formatter.
Comment #92
BerdirThe test fails are because of a bug in EntityReferenceFormatterBase::prepareView() but anyway, AuthorFormatter is something we need to keep I guess, except we add a rel option to the EntityReferenceLabel formatter...
So, back to #88, for decision about depending on entity_reference or not. We did introduce the date alters to avoid a dependency on datetime in node.module, but that was before we removed it as a testing install profile dependency.
Comment #93
andypostcould use "string_long" after #1856562: Convert "Subject" and "Message" into Message base fields
Comment #94
Wim Leers#81.14: It's not a performance optimization; it's to ensure that cache tags are bubbled up.
#84: "author widget": IIRC this was necessary because of the special case that we have for
User
entity references: if blank, use the anonymous user. And I think that on top of that there's also<name>
being allowed instead of only<name> (<ID>)
. In other words: special snowflake behaviors on top of the basic behavior, for a better UX.#85: indeed.
Comment #95
andypostsorry
Comment #96
andyposta bit of clean-up
Comment #97
andypostSorry for noise, wrong issue #2227503: Apply formatters and widgets to Comment base fields
Comment #100
Wim LeersStraight reroll of #88; #88 was broken by #2061977: Replace user_access() calls with $account->hasPermission() in all core modules except user.
Comment #101
Wim LeersThis patch was RTBC'd 1.5 months ago in #60. Then it was broken twice by commits. Then tstoeckler came in and asked for things that didn't exist until very shortly before the patch was RTBC (
<entity type>::create()
etc.) — which is fine, but not very helpful for a patch that easily conflicts with other commits.Then I was working on a big beta blocker ("menu links home stretch"), which didn't leave me any time to reroll this one. Berdir then rerolled this and addressed tstoeckler's feedback (thanks! :)).
And now it seems this issue is blocked on deciding whether it should rely on the standard entity reference widgets. I'd love to see that happen also. But, that is nontrivial due to the special behavior for the author autocomplete that we've had so far (see #85 and #94). On top of that, I think it's a nice-to-have. Please let us get this in, it converts all node base fields to use formatters and widgets. Once we have that, it's much easier to move from there to the standard entity reference widget.
Please let us get this in ASAP. We've been rerolling this for almost 4 months. It took us >2 months to get it to green. Since May 31, it's only been broken by commit conflicts and it's only been criticized for not having things that have only become possible after May 31. Let's not let Perfect be the enemy of Better. Let's get this in, and then make further incremental improvements.
Comment #102
BerdirSee the discussions above. adding the custom widgets there results in them showing up the UI, which makes no sense as they need configuration that can't be entered in the UI to function. So we would need to introduce a system to hide widgets like we have for field types.
The same is true for the author formatter. Not sure what to do there.
Also, I don't really see the different anonymous behavior as a problem, there was exactly one test fail due to that. And as discussed, entity reference supports just "Username" too, or we would have had more fails.
Comment #104
Wim Leers#2226063: Merge ListBooleanItem from options module into BooleanItem landed; now we need to reroll this, and it'll become a bit simpler yet again :) Berdir++
Comment #105
YesCT CreditAttribution: YesCT commented#2301313: MenuLinkNG part3 (no conversions): MenuLinkContent UI adds a todo related to this. just noting.
Comment #106
BerdirRe-roll.
Most of those fails were due to the missing + on the revision_log addition I think.
Also removed the new Boolean widget, using the existing one now. Removal is not visible in the interdiff, the resulting change for the display options is.
Comment #108
BerdirRemoved the no longer used widgets, added schema for datetime_timestamp, fixed other missing "+"'s and updated tests for the new checkbox widget.
Comment #109
yched CreditAttribution: yched commentedThat issue has been closed as "cannot reproduce". So we should try to find out if there really is an issue ?
(same in DateTimeTimestampWidget)
Will this still work when the datetime.module is enabled and changes the 'created' widget to DateTimeTimestampWidget ?
So this is the last problematic formatter - it is eligible and will show up in the "Manage display" UI for all entity_ref fields, but only makes sense for entity_ref fields on users (also, the current code will probably have weird effects / break when used on a non user entity_ref).
Since the 'uid' base field is not set to have its display configurable in the UI, maybe we don't need a formatter for this, and could stick with hardcoding the theme_username() in template_preprocess_node() from the raw field value ?
That would remove the need for the custom field--node--uid.html.twig.
(maybe we should do something similar for 'created': render the value with a formatter in template_preprocess_node(), and lose the speciic field template ?)
(Also, that 'uid' field should be renamed 'author', really - but that's probably be too big for this issue here, opened #2313799: Rename node 'uid' field to 'author')
Comment #110
BerdirRe not using formatters.. I think using formatters is Wim's main motivation for these issues, as it allows editor/quickedit to work with those fields, that is not possble if we keep the manual output.
3. Specifically for this one, I was hoping that we could use #2204325: The "rendered entity" formatter breaks for entity types with out a ViewBuilder to only display that option for references to user entities. For those, having a formatter that displays the reference with a rel="author" seems like a useful thing? Could use your opinion on that issue.
Comment #111
yched CreditAttribution: yched commentedFormatters / quickedit: ah, sure, silly me.
Will look at that other issue.
Comment #114
m1r1k CreditAttribution: m1r1k commentedRerolled. The problem occurred because of introduced FormStateInterface in datetime.module file.
Comment #116
m1r1k CreditAttribution: m1r1k commentedrererolled
Comment #118
m1r1k CreditAttribution: m1r1k commentedComment #120
m1r1k CreditAttribution: m1r1k commentedDefinitely the last fix of FormStateInterface.
Comment #122
andypost2 more left
created could be accessed via populated entity, is not it?
Comment #123
m1r1k CreditAttribution: m1r1k commented1), 2) Fixed.
Also i've removed custom validation of created and author fields from NodeForm as they are fields now and have own validation.
Comment #125
m1r1k CreditAttribution: m1r1k commentedAdd default value callback for uid basefield.
Fixed node.js script.
Fixed NodeForm->buildEntity method.
Comment #127
m1r1k CreditAttribution: m1r1k commentedAnother attempt :)
Comment #129
andypostA bit fixed interdiff from @m1r1k
Revision log now has field access control after #2098355: Missing default access for all node fields
Comment #131
m1r1k CreditAttribution: m1r1k commentedComment #133
m1r1k CreditAttribution: m1r1k commentedPrevious patch contained one useless modification and interdiff was invalid. Here is an update.
Comment #134
yched CreditAttribution: yched commentedNow that #2204325: The "rendered entity" formatter breaks for entity types with out a ViewBuilder was committed, AuthorFormatter should use the added isApplicable() method to only show on entity ref fields targetting users.
Comment #135
cbr CreditAttribution: cbr commentedrerolled, will be working on #134
Comment #136
cbr CreditAttribution: cbr commentedComment #137
Wim Leers@m1r1k et al: As you can tell, it's quite noisy to upload patches just to see what testbot's verdict will be. For complex patches (like this one), where the likelihood of test failures is high, we generally prefer to post patches to "helper issues" (like #2268971: Helper for #1805054), and then once we have a green patch, we post it to the actual issue.
Comment #139
cbr CreditAttribution: cbr commentedAdded isApplicable() method to only show on entity ref fields targeting users and provided test coverage for this in EntityReferenceAdminTest.php.
Comment #140
andypostShould be just:
return $field_definition->getSetting('target_type') == 'user';
Leave an empty line between end of method and end of class definition
https://www.drupal.org/node/608152
Comment #141
Berdir1. No, the target_type is a field storage setting, that is correct.
Comment #142
BerdirMissing the @inheritdocs docblock.
Comment #143
andypostFixed a few doc-blocks, suppose we need @yched to fire rtbc
Comment #144
yched CreditAttribution: yched commentedI don't think points 1 and 2 from #109 have been answered yet ?
Comment #145
Wim LeersBesides #144, there's one more thing: since #2098355: Missing default access for all node fields landed, we should be able to remove the
#access
stuff inNodeForm
; which means almost everything of the node form will be generated!Comment #146
yched CreditAttribution: yched commented#145 means NW ?
Comment #147
BerdirAlready working on this and the @todo's.
Comment #148
yched CreditAttribution: yched commentedAlso, I'm thinking we should probably get #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI in first, it clarifies the roles for string* anf text* field types and the corresponding widgets (which will likely clash with what is made in this patch)
Comment #149
BerdirThis seems to work, the reason the timestamp massaging didn't work I think was there was a left-over code in buildEntity() that didn't work anymore as expected because created was always there but as "array('value' => '')", so empty said FALSE.
That said, validation is a problem now. With the core timestamp widget, we get that insanely stupid "This value should be of the correct primitive type.", and with datetime, weird stuff happens, we have no place where validation is done anymore, so it tries to save NULL if I pass around NULL, and in the current code -1. It is also, already in HEAD, easily possible to crash it by either selecting 2050 or 1900 in the widget because the value is then too large. Sounds like we need a max/min validation on the timestamp, but that again will result in a meaningless validation error. :(
Comment #150
Berdir#148: The only place where that is relevant, I think is the revision_log widget, and I just changed that to string_textarea now, forgot to do that before. Now the other issue shouldn't affect this anymore?
Comment #153
yched CreditAttribution: yched commentedRe #150: yay! Then we should also remove the hunk in TextareaWidget that adds string_long as an applicable field type - it has no real sense, and is the part that would clash with #2313757: Remove text_processing option from text fields, expose existing string field types as plain text in UI (at least conceptually, if not patch-wise)
Comment #154
BerdirHm, massageFormValues() somehow gets inconsistent values...
Comment #156
BerdirRe-roll.
Comment #157
yched CreditAttribution: yched commentedI'm still reviewing, but reacting to the last couple comments:
You mean when #access is TRUE or FALSE ? Ouch, yeah, there might be an issue with here.
That would require investigation in its own issue. I opened #2326533: The structure of values received in WidgetInterface::massageFormValues() varies depending on #access TRUE/FALSE (@Berdir, please confirm/adjust the issue summary there).
Meanwhile, we can keep the workaround you put in place with a @todo on the above ?
Oh ? That feels a bit backwards, deriving the #access of the group from the #access of another (arbitrary ?) field rather than on proper permissions ?
Validation issues raised in #149:
Not sure I completely follow, and so not clear on the possible way forward.
- Is that a consequence of moving to massageFormValues() ?
- "This value should be of the correct primitive type": don't we have an issue to allow constraints to pass more telling fail messages ? If so, isn't just a @todo on that known issue ?
- "with datetime, weird stuff happens, we have no place where validation is done anymore"
Sounds bad - where was this validation done so far ?
- "Sounds like we need a max/min validation on the timestamp, but that again will result in a meaningless validation error. :("
Would that then be the same "@todo [custom validation message issue]" than above ?
"false non-empties" are a real pain :-/. We bump into them less often because we have sprinkled random ->filterEmptyItems() calls all over the place, but they still appear very easily (just ->get(non existent delta) and it creates one) and so we're still very brittle on that.
I still think we should fix #2164601: Stop auto-creating FieldItems on mere reading of $entity->field[N], but I got a bit stalled there... (also, the code was a bit of a shoot in the dark)
Comment #158
Berdir1. Yes, I think that is exactly the problem I saw. Will clarify the @todo and reference that issue.
2. Yes, I'm not sure about that. The revision_log field is visible when you have permission to toggle new revisions *or* when you don't but the new revision checkbox is already ticked. So it's more a UI thing than something else. Not sure that is something that belongs in field access? Not showing it is more a UI thing than anything else, it's not that you wouldn't be allowed to write in it, it just won't be used and would be misleading.
3. Date fields on the node form are a huge mess, there are existing issues, see #2031183: Improve test coverage for node authored on widget and #2315023: Saving a node without having the datetime module enabled updates the created field always ( think they are duplicates, more or less). And this makes it worse, part of the validation that we had is removed in favor of non-existing field validation ( no max/min for what we can actually store, only the weird integer scalar check) because there's quite the mismatch between the field type (timestamp) and UI which shows date strings, which we can only convert to a timestamp if it's valid in the first place, we can't validate a timestamp to see if it's a valid, so that validation has to happen in the UI/widget.
At some point, node.module depended on datetime.module for that widget, we changed that to be optional as node was then still part of testing profile, and we wanted to avoid enabling datetime in the testing profile. This has been a mess since at least then, probably longer. That changed, so I'm considering to drop that and just rely on datetime.module again.
4. I know that issue, but I don't think it would help here. There actually *is* a field item (after all, we just displayed a widget for it), but the user left it explicitly empty. As mentioned, I think the what Wim saw as massageFormValues() being broken was really the combination of massageFormValues() and that removed snippet there, which was the last line of defence against an empty created date that then no longer worked.
Comment #159
BerdirReroll and updated the @todo.
Comment #160
yched CreditAttribution: yched commented- Regarding 2. and
So $form['revision_log']['#access'] comes in populated with what gets assigned by the field permissions, right ?
So what this does is discard field permissions to always show the input field if isNewRevision() ?
Yeah, not sure if that "isNew()" behavior should be baked in the 'revision_log' field permissions in NodeAccessControlHandler::checkFieldAccess(), or be adjusted directly in the form as "UI thing only".
Then again, the current code means we write a field value for which the permission says "no write grants" ? Isn't that weird ? Don't we want to enforce that same behavior on REST writes ?
At any rate, if we do leave it in $form, this snippet might deserve a comment, as that's not too straightforward on first read (I misread it at first)
- Regarding 3. and date / validation messages
Yeah, switching the widget depending on whether datetime.module is enabled feels really awkward. Not sure whether the datetime widget can/should be moved to Core, or if node should depend on datetime.module, but the current situation feels like a WTF.
Then again, even if node depends on datetime, doesn't it just leave the issue for other entities that also want to store creation dates ?
All in all, not too sure of the way forward there...
Comment #161
yched CreditAttribution: yched commentedActually, yes, not sure why DateTimeTimestampWidget can't move to Core ?
Comment #162
BerdirHere are the three possible states for revision_log:
- administer nodes permission: #access TRUE, visibility with #states based on revision checkbox.
- new revision enabled by default, no administer permission: revision_log always visible
- new revision disabled by default, no administer permission: revision log always hidden
We can probably implement that logic in field access, if we want to do that. not 100% sure, but I can try. It is not possible to create new revisions over rest anyway, that's kind of the bigger problem ;) (or do anything with revisions)
3. datetime.module currently provides a lot of elements and theme functions. both for form elements/editing and displaying. While element stuff can now actually live in Core, theme functions is afaik still a bit awkward (=system.module AFAIK). comment, unlike node, depends on datetime.module.
Comment #163
yched CreditAttribution: yched commentedThen would it make sense to have:
- field permission = ('administer nodes' perm) || (new revision enabled by default for the node type)
- UI #states sugar in the $form to only show the textarea when it actually makes sense (if the revision checkbox is (checked) || (hidden because forced to "checked")
?
datetime.module: sure, moving it to Core sounds like a valid but non-minor effort, but I was talking specifically about the DateTimeTimestampWidget - I see nothing in there that couldn't be moved to Core as is ?
Comment #164
BerdirIt uses #type datetime, which is defined in datetime.module. It would be much easier to move that now that hook_element_info() are plugins, but would still involve quite some callbacks and stuff I guess.
The #states stuff already exists, so that access might work yes, I'll try it out.
Comment #165
yched CreditAttribution: yched commented#type datetime - doh, right, silly yched.
Oh gee. Yes, moving that #type to an Element class would sure help make things clearer, but that's for another issue.
As a side note, opened #2327363: Move datetime_datetime_widget_validate() to DateTimeDefaultWidget::massageFormValues() (but that's not really related)
Comment #166
BerdirTrying the revision_log access changes, with tests.
Comment #167
yched CreditAttribution: yched commentedFYI : opened #2328061: Move datetime's FormElement #type classes in Core
Comment #168
BerdirNice, Tim being awesome again :)
Moved the datetime widget to Core\Datetime (not 100% sure if it should be there or in field, but I think it makes sense there, but can't be the default then), removed the other one, removed the alter stuff.
Improved validation by limiting the years to a safe value (can't make it exact dates, so playing safe), and also added a range validator to the timestamp item.
Comment #169
jibranWhat is the reason for this change?
Comment #171
BerdirThe reason for that change is that "Anonymous (0)" results in $value = 0, which is a valid result, so we need to do a type safe check.
Went through the patch again, cleaning up things. A bit confused by NodePreviewTest, I thought that I've fixed that properly before, the right fix is not to give the user admin permission but to configure the node type properly, instead of the bogus code that was there before.
Similar for EntityTranslationFormTest, I just reverted that to the original test and removed the bogus :? FALSE (because it's never used anyway with the old code and it's all working just fine. I suspect the changes there date back to attempts to create a widget for langcode.
Comment #172
yched CreditAttribution: yched commentedStill have a couple files to review, posting what I have for now (minor stuff)
minor: each var could be declared above the hunk that actually tests it ?
Comments needs an update
The other replacements in that file use ->toArray() rather than ->getValue() ?
Apparently the var is not used anymore by the code below it, should be removed ?
Comment #173
Berdir1. Yes, moved it.
2. Updated.
3. Switched to toArray().
4. I assume you reviewed the second last patch? I reverted almost all changes in that test in the last patch, so it is no used again.
Also cleaned up NodeController because the uid is now set by default. This means that this essentially solves #1979260: Automatically populate the author default value with the current user too. It's not very nice, but it works.
Comment #174
yched CreditAttribution: yched commentedOK, finished reviewing at last :-)
Core/Datetime does provides the base FAPI #types, but Core/Field provides the TimestampItem field type and the TimestampFormatter - would it be really problematic if TimestampDatetimeWidget was in Core/Field too ?
Would mean Core/Field contains dependencies on Core/Datetime, but the patch currently does the opposite, so it would not really be worse ?
Would be more natural IMO, and let TimestampItem / CreatedItem use it as their 'default_widget' ?
The datetime_timestamp widget is in Core, so we shouldn't be adding its config schema in entity.module ?
does this need to be re-evaluated in light of #2273277: Figure out a solution for the problematic interaction between the render system and the theme system when using #pre_render ? @Wim ?
Why don't we add setDisplayConfigurable('form', TRUE) ?
We want to allow rich contrib entity_ref widgets there, don't we ? (can be a followup if we suspect dragons...)
Moving the parent:: call at an arbitrary position in the middle of the method is a bit surprising, might be worth a comment explaining why it's done there ?
(also, can't it move at the beginning instead ? I can see the "Create the "advanced" vertical tabs before building the form, so that field widgets may detect its presence and choose to live there." part, but I'm not sure how widgets could actually do that ?)
Why don't we just set those as the field labels in the base feild definitions ?
This is only done in bartik, not in node.module/templates/node.html.twig ?
Comment #175
Berdir1. Yeah, I'm not sure there, would be great to get some other opinions on that. Core\Datetime also has an entity type now, so it depends on Core\Entity already. I don't really care, I'm can live with both options.
2. We can move it to core when the entity.module no longer required issue is committed, which will also move the others. However ,right now, it extends from a type that is in entity.schema.yml, so that seems like the right place ATM. Hopefully the right place for a very short time :)
3. Leaving that to Wim.
4. One reason there are groups. While we can make it configurable and switch the widget, we can't movei t outside of those groups, as the manage display screens have no understanding of that.
5. I had to move it a bit after the preview patch, not sure why exactly it's not first. I can try to move it first, we'll see what happens then.
6. Was wondering that too, fine with me, I don't think t('Created') is a good label :)
Comment #176
yched CreditAttribution: yched commentedYes, but isn't that also true for other fields where we allow teh widget to be configurable ?
No reply on 7 ?
Comment #177
moshe weitzman CreditAttribution: moshe weitzman commented3. It used to be a really bad idea to render stuff in a preprocess since the cache tags would get dropped on the floor. Now, it is still discouraged but not as big of a problem as the cache tags are passed up the stack. So, this issue doesn't need to tackle the bad form. Proceed!
Comment #178
yched CreditAttribution: yched commented@moshe / @Wim: Is the second param on drupal_render() still needed though ?
(just wondering)
Comment #179
BerdirFixed the parent call, labels and node.html.twig.
Comment #180
Berdir@yched: Yeah, created is set to TRUE. I don't know why some are and others not, maybe uid wasn't because it was a custom widget?
Comment #182
effulgentsia CreditAttribution: effulgentsia commentedFor now, yes. If you invoke drupal_render() from within rendering (i.e., in #pre_render, preprocess, or theme function), you need to pass in TRUE. If you don't, then if the element has a #post_render_cache on it, it'll get run twice. There might be places not passing it in and still working, either because they happen to not use #post_render_cache or because the #post_render_cache function happens to be idempotent, but where possible, we shouldn't rely on those accidents. Perhaps some day, we'll find a way to remove that parameter.
Comment #183
effulgentsia CreditAttribution: effulgentsia commentedActually, it's worse than that. When you call drupal_render() without setting the 2nd param to TRUE, its #post_render_cache functions will run, and then a parent element might have #cache set, and we'll end up render caching the expanded HTML, which would result in a cache poisoning bug.
Comment #184
yched CreditAttribution: yched commented@effulgentsia: thanks for the explanation. That second param (or rather knowing when to set it or not, the effects of not setting it when you should and reversedly) sounds a bit worryingly brittle, but well, out of scope here.
@Berdir : yeah, IIRC at some earlier point the patch used a dedicated AuthorWidget for node.uid because the regular entity_ref widgets wouldn't work. I think this was fixed meanwhile, right ? So we could relax the constraint on the widget now ?
@Wim, does that ring a bell ?
Anyway, as I said, this can also be an easy followup. This is RTBC for me when it's green again. Thanks a lot @Berdir & @Wim !
Comment #185
BerdirIt's really hard to stop here. Had a look at the ui configurable thing and noticed an interesting problem with the current code: If you hide such a configurable widget, the code blows up because we did an unconditionaly += on the form array. Replaced that with isset checks now and made uid, stick and promote also UI configurable.
The reason I added it for those too is that I basically sold this issue as an alternative/additional fix for #987242: The "Promoted to front page" checkbox doesn't do anything if the /node front page listing isn't used: if you don't need promote/sticky for some node types, just hide it in the form display.
Also reverted the parent::form() call change, there's some preview $form_state replacement going on above those lines that must happen first.
And while doing that, I also noticed the bogus anonymous user name setting and logic for it in node.js, removed that too. And when testing that, I noticed that the sticky/promoted summary includes the description, not sure if caused by the change to a widget or not. There's more cruft in there, like translate options that were removed years ago. (We don't show this stuff anymore in seven, but it is still there...)
But as I said.. must. stop. making. more. changes..
Comment #187
effulgentsia CreditAttribution: effulgentsia commentedAgreed. The best answer at this time is: "don't call drupal_render() explicitly once you're in the rendering flow; leave it to Twig to call it via {{ foo }} only". To that end, do we need to call it in:
Any reason we can't leave them as render arrays like:
?
Comment #188
Berdir@effulgentsia: Because the output is a translatable string, and that apparently doesn't support passing through render arrays. Just tried it.
Sounds like a good follow-up to fix that, though? Even wondering about just using created/uid directly, right now, we render it even when we later on set display_submitted to FALSE. So maybe we can move all of this into the template (given that we already moved the Submitted by string there too) and treat it similar to links? Once it actually works, I mean.
Looks like we were missing the schema for the boolean_widget...
Comment #190
BerdirThe weight needs to be unique for configurable widgets, or EntityDisplay will re-order and change the default weights.
Comment #192
yched CreditAttribution: yched commentedStill don't get that comment :-) I don't see generic widgets assuming the presence of an "advanced" tabs group in an entity form ?
Comment #193
BerdirNeither do I :) I find it strange too, to be honest, especially given that seven doesn't even really render that stuff anymore. Removed it again, but left it in that order. I thought it's a comment that was already in HEAD, but that's not the case.
Also, more weight juggling. Weight needs to be unique, above 0 and also not conflict with any dynamic fields (for example, comment has a hardcoded default weight of 20 for the comment field I think).
Comment #194
Fabianx CreditAttribution: Fabianx commentedI opened #2331393: drupal_render() recursive parameter usage is unclear, safe default should be drupal_render(x) as follow-up to the drupal_render($x) vs drupal_render($x, TRUE) issue.
@Berdir will open a follow-up to ensure twig trans does support render arrays (which is simple todo if we take the performance hit of another function call, for sake of DX we probably should).
Comment #195
Wim Leers#174.3/#178:
No, this is fine :)
(As confirmed by moshe in #177, and further explained by effulgentsia in #182.)
#174.6:
Because "Authored on" doesn't make sense in contexts outside of the node form. I agree with #175.6 that "Created" doesn't make sense, but "Publication date" (which I think was in this patch at some point) would be better. But that doesn't change the fact that "Authored on" only makes sense for the node form.
I think I might have copy/pasted this from elsewhere in Drupal core when initially working on this patch.
Removing this is probably okay now.
What's left before this can be RTBC? :)
Comment #196
yched CreditAttribution: yched commentedI don't get that. It also works as a label on display (even though node.html.twig used a hardcoded string rather than the field label).
So where doesn't it work ?
Comment #197
Wim LeersOk then, let's change it.
(Months ago this seemed a bad idea, but the way you put it here, it seems fine. It's easy to change later on if we find it to be problematic.)
Comment #198
BerdirWhich is already the case in the patch now, so as far as I know, there is nothing left to do...
Comment #199
Wim LeersYesterday night, #184, yched:
I also did a final round of manual testing just now. Everything works fine, including in-place editing.
Thanks for pushing this to the finish line, Berdir! :)
Comment #200
star-szrThere shouldn't be a space between
<span
and{{
, Attribute prints its own whitespace so that you don't end up with things like<span >
when there are no attributes.Fixed here because it's removing two characters. No commit credit necessary, this just came across my desk because it's tagged Twig ;)
Comment #201
joelpittetSuper minor twig coding standards fixes. Also wanted to get the comments to an even 200:P
@see https://drupal.org/node/1823416
Crosspost with @Cottser 201:(
Comment #202
Fabianx CreditAttribution: Fabianx commentedRTBC +1 for #201
Comment #203
BerdirOpened #2334319: {% trans %} does not support render array and MarkupInterface valued placeholders. Needs more details on how to do that.
Comment #204
alexpottThis looks great. Just one thing I noticed.
Looks like these should be in the options group not author.
Comment #205
BerdirOuch, that happened in one of the last changes when cleaning up the form alters, too much copy paste.
Fixed, also visually verified.
Hope it is OK to set this right back to RTBC.
Comment #206
alexpottCommitted 2b0be1d and pushed to 8.0.x. Thanks!
Comment #208
Wim LeersWonderful! :) I just installed D8 and for the very first time, I'm able to in-place edit node author and date. In-place editing of nodes is now finally working as intended!
Comment #209
Wim LeersBut this did cause a UX regression for changing the author — the UX of the widget is inferior to what we had before. See #2335321: Node author widget UX is lacking (also affects Quick Edit's UX). Still, that's a small price to pay considering the improvements this patch brings, and fixing that UX problem will also benefit all other uses of the entity reference autocomplete widget! :)
EDIT: oops — I linked to this issue :P
Comment #211
David_Rothstein CreditAttribution: David_Rothstein commentedA followup to add this removed wording back to the node form is here: #2350013: Descriptions for Promotion Options are too technical
Assuming that was a mistake and the node form wording was not intentionally changed in this issue.