Problem/Motivation
In order to do better profiling, we need good placeholder data in core. We're seeing that at #2308683: [meta] Create a 'Profiling' install profile, for testbot-powered simple profiling and easier local profiling. We also need placeholder data for for stubbing out sites as they are built.
Proposed resolution
Put parts of devel_generate module in core. This issue is about letting Field types generate their own sample values.
Remaining tasks
None.
User interface changes
None.
API changes
Adds a generateSampleItems() method to FieldItemListInterface, and generateSampleValues() static methods to *Item classes.
Comment | File | Size | Author |
---|---|---|---|
#60 | interdiff.txt | 2.32 KB | moshe weitzman |
#60 | generate_placeholder-2320157-60.patch | 40.88 KB | moshe weitzman |
#55 | generate_placeholder-2320157-55.patch | 39.37 KB | moshe weitzman |
#53 | interdiff.txt | 548 bytes | moshe weitzman |
#53 | generate_placeholder-2320157-53.patch | 39.37 KB | moshe weitzman |
Comments
Comment #1
moshe weitzman CreditAttribution: moshe weitzman commentedLinkItem class now has generate(), along with minimalist testing. I think validity testing is the responsibility of Field Validation system which can be a followup. Field validation applies to user submitted data just as much as generated data.
Comment #2
moshe weitzman CreditAttribution: moshe weitzman commentedNow with the right patch
Comment #3
penyaskitoI like the idea.
Comment #6
moshe weitzman CreditAttribution: moshe weitzman commentedFix method name to fix test fatal.
Comment #7
moshe weitzman CreditAttribution: moshe weitzman commentedComment #8
swentel CreditAttribution: swentel commentedI like the idea. Can't really think of a reason why we wouldn't do this.
2 small things in docs found.
Documentation needs some cleanup
80 chars
Comment #9
moshe weitzman CreditAttribution: moshe weitzman commentedFixed those nits - thanks.
Now with date, taxonomyRef, entity_ref fields implementing generate() along with tests for each. Feedback welcome.
Comment #10
dawehnerGreek != Latin.
As far as I remember back from school these words are latin.
Comment #11
penyaskitoSome nitpicks.
Even if it is obvious, I guess we should add docs for $word_count.
Could we rename $title to $uppercase?
$referenceable?
Spaces before and after -.
Comment #12
penyaskito@dawehner: http://en.wikipedia.org/wiki/Greeking
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedNew patch with all field types implemented. I an now both validating and saving the test entity and have verified that Field constraints are being applied.
Comment #17
moshe weitzman CreditAttribution: moshe weitzman commentedHopefully fix fails.
Comment #19
Wim LeersLooks simple & sane. Comes with test coverage. Good to go AFAICT.
That leaves only nitpicks. I already fixed most, which leaves only a few:
This confused me very much, just like dawehner in #10. #12 contained the answer. Perhaps link to http://en.wikipedia.org/wiki/Greeking here, to avoid future issues being opened about this? :)
(Or, alternatively, something like
createRandomPhrase()
?)Doesn't "expected" imply the possibility for a deviation, i.e. "could be more, could be less"?
Wouldn't "desired" or some other adjective be more accurate?
Wow, cool!
Comment #20
Wim LeersComment #21
moshe weitzman CreditAttribution: moshe weitzman commentedFixed Wim's items from #19 and added PathItem::generate()
Comment #22
moshe weitzman CreditAttribution: moshe weitzman commentedComment #23
moshe weitzman CreditAttribution: moshe weitzman commentedWoops, I had missed Wim's patch. Thats incorporated here. See his interdiff in #19.
Comment #24
penyaskitoRenamed
$title
parameter to$capitalize
. Some coding standards nitpicks.This patch is adding calls in
Drupal/Component/Utility/Random
todrupal_*
functions, which makes me think that we should move it toDrupal/Core/Utility/Random
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedOr, generateImage() could return the image handle, and the directory/file creation could be moved to FileItem::generate().
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedMoved the drupally bits of of Random::generateImage as per effulgentsia in #25. I moved too quickly and did a --amend do I can't provide an interdiff. The only changes were the top and bottom of Random::generateImage() its call in ImageItem::generate().
Comment #27
penyaskito#26 misses changes at #24.
Comment #28
moshe weitzman CreditAttribution: moshe weitzman commentedWoops, now incorporated #24. Sorry about that.
Comment #29
yched CreditAttribution: yched commentedWill try to look at the patch soonish.
Meanwhile, nitpicky, but I'd like if we could find a more specific method name for"generate()" - somthing that conveys the idea that it's about dummy sample content. Was clearer while those lived in *devel*_generate.module, much less if it's a generate() method among the other ones in FieldItemInterface.
Would generateSample() work ?
Comment #30
Wim Leers#29: I felt the same way, but couldn't think of something better. I kind of like
generateSample()
. Or maybesetRandomValue()
, orsetSampleValue()
?(But let's be careful to not over-bikeshed this. :))
Comment #31
yched CreditAttribution: yched commentedWell, nothing forces the value to be "random", right ?
Also, if anything, it would be more a getter than a setter ?
I think generateSample() still has my vote. Or (question to native english speakers) does "sample" sound weird if used as a standalone noun ? generateSampleValue() then ?
Comment #32
effulgentsia CreditAttribution: effulgentsia commentedWell, as currently implemented, generate() performs a $this-setValue() on what it generates, so it's a setter. However, what about renaming it to
generateSampleValue()
and making it return the property values array only, requiring the caller to then also call->setValue()
with that result if that's what it wants to do? Pro: makes each step more explicit. Con: for the common case, it requires the caller to call two methods instead of one.Related, I think we should remove the "generate" and "create" prefixes within the methods being added to the
Random
class, since for that class, it's already implied, and it would better match the existing methods in that class.Comment #33
yched CreditAttribution: yched commentedOh - I should have actually looked at the patch :-p
I'd tend to agree with one dedicated method to generate the raw values, and them being set separately by the caller. Stricly speaking, the "sample generation" is the logical unit each field type needs to override, the "set" part is common.
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commentedRenamed method to generateSampleValue() and leaving setValue() to the caller.
Comment #35
yched CreditAttribution: yched commented- generate / create ?
The existing methods in the Random class use no verb at all (->name(), ->string()...), we should be consistent with what's already in there ?
- The relation between the methods is not what you'd expect:
generateParagraphs() uses createSentence(), but createSentence() does not use generateWord().
--> Maybe add "latin" in the method names for paragraphs and sentences, and leave word as a separate, unrelated thing ?
- about "word": the class already has string() and name(), do we really need a 3rd variant ?
If so, it should be added next to the other ones, rather than in the middle of the "latin" stuff it has no connection with.
IMO we could add that it's the responsibility of the implementation to respect the field settings and produce values that will pass validation.
Also, could we add it somewhere else than between delete() and deleteRevision() ? :-p
Missing empty line after last method
that kind of code would look slightly simpler if the returned array was inlined directly in the return statement.
Matter of taste, feel free to ignore.
- Extra empty line
- Method misses visibility - protected ?
Will that work if 'min' is not set ?
if 'max' is 0 ?
lacks visibility
Not sure what will happen then ?
leftover - thus lacks test coverage ?
We generate a sample alias using capitalized sentences ? Surprising ?
Also, is that enough to provide a sample PathItem ? No pid ?
ChangedItem
CreatedItem
LanguageItem
MapItem
StringItem
StringLongItem
TimestampItem
UriItem
UuidItem
[edit: removed a couple "plz move generateSampleValue() implementation to the end of the class" items, pointless]
Comment #36
yched CreditAttribution: yched commentedYeah, that's tedious :-/
We could add FieldItemList::generateSampleItems($count), that would take care of creating the items and setting their sample values ?
-->
$entity->field_foo->generateSampleItems(3);
Comment #37
yched CreditAttribution: yched commentedWhich also makes me think that FieldItemInterface::generateSampleValues() should IMO be static, with a FieldDefinitionInterface param : you shouldn't need to have an instanciated Item object in order to create a sample one.
FieldItemList::generateSampleItems() would then figure out the item class, call the static method to get the sample values, and call $this->createItem($delta, $sample_values).
Comment #38
yched CreditAttribution: yched commentedBTW, having the ability to generate sample field values in core means we could provide "entity previews" from the "Manage display" UI screens (could be in contrib, or even in core 8.1.x ?)
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commentedAttached patch implements everything mentioned by yched in the last 3 comments. I only omitted 3 items (see below). Hopefully I got the static method stuff right. Sorry about the missing interdiff. I am trying to improve my habits.
I have one problem which is causing fails in EntityReferenceItemTest, TaxonomyTermReferenceItemTest, and BooleanItemTest. Now that generateSampleValue() is called statically, I can't use $this in a line like
$referenceable = $manager->getSelectionHandler($field_definition, $this->getentity())->getReferenceableEntities()
. is there some way I can get an $entity or avoid the call to getSelectionHandler()?I thought about this and I don’t see that adding Latin to the name as a net win. The two method names just get longer. Also, the behavior of word() is different enough from string() and name() to justify its existence. I was a bit surprised as well.
The pid gets created during \Drupal\path\Plugin\Field\FieldType\PathItem::insert
I just focused on the field types that are likely to be added by users to ContentEntities. What would be the value to adding generation for ChangedItem, uuid, etc. I don’t mind doing it, but lets identify which ones and why.
Comment #40
tim.plunkettYou can always create an interdiff after the fact.
Comment #42
yched CreditAttribution: yched commentedThe $entity would need to be passed as a param to the generateSampleValue() method ? It seems right that the method is "generate sample values for this $field_definition in this $entity" ?
FieldItemList::generateSampleItems() can encapsulate this by taking care of passing $this->getEntity().
Well, node->created uses CreatedItem, node->title uses StringItem... - how can we generate sample nodes if we can't generate default values for those ?
Not saying that this issue here has to cover *all* core field types before it gets in, we open followups for the less used ones, but maybe at least covering the field types used by the "visible" base fields of core entity types (or maybe just nodes or users ?) would be worthwhile ?
Okay - can we then just move the word() method out of the sentence() / paragraphs() group, and next to string() / name() ?
And maybe add a comment clarifying how it differs from those two ?
Comment #43
moshe weitzman CreditAttribution: moshe weitzman commentedOK, now passing $entity as suggested. Thanks.
Moved word() and added some Doxygen:
* Generate a string that looks like a word (letters only, alternating consonants and vowels).
I added StringItem::generateSampleValue. I'm skipping Changed field type since that gets set for all entitites in ChangedItem::preSave
@TimPlunkett - nice tip!
Comment #45
effulgentsia CreditAttribution: effulgentsia commentedPassing $entity here doesn't make sense to me. At the very least, we should make it optional. But do we want it at all? For ER,
getSelectionHandler()
has it as optional, which means we can choose to make generateSampleValue() not pass it in. For ListItemBase, we have #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration open and critical, so we can punt its implementation until then. For AllowedValuesInterface field types in contrib, we have #2305397: Field type (item) classes implementing AllowedValuesInterface::getPossible(Values|Options)() is incompatible with Views and possibly other use cases, but even without that, each one can implement its own workaround as discussed in that issue.For use cases where we're generating sample values to add to a new entity, there's no information in $entity that would be useful to the implementation. So the only logical reason for passing it in would be if we wanted to support adding sample values to an entity that already exists, and therefore, might already have meaningful (non-sample) information on it that needs to affect the sample value we want to generate: is that a real use case we want to support?
Comment #46
yched CreditAttribution: yched commentedThe reasoning makes sense - still, the entity type / bundle might need to affect the sample value ? Can't really think of an actual example though.
Yeah, maybe not passing $entity, and punting on #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration for ListItemBase::generateSampleValue() is simpler.
Comment #47
moshe weitzman CreditAttribution: moshe weitzman commentedNo longer passing $entity and commented out ListItemBase::generateSampleValue() with a link to #2238085: [regression] options_allowed_values() signature doesn't allow for Views filter configuration. Does that sound like a good way to proceed for now?
Comment #48
effulgentsia CreditAttribution: effulgentsia commentedWorks for me.
Remove 2nd argument.
Is there a reason to save the entity in the tests? I think validating should be sufficient.
Since we don't know what the implementation will need to be until that issue lands (but we know it won't involve having a $entity), maybe let's remove this. We can leave the function wrapper and @todo though.
Comment #49
effulgentsia CreditAttribution: effulgentsia commentedWe can get the entity type from the field definition. IMO, bundle should not affect any implementation directly, only implicitly via the field definition. However, one use case I thought of for passing $entity is if we ever want sample values to depend on the entity's language. But I suggest leaving that out for now and waiting for a use case that actually requires that.
Comment #50
effulgentsia CreditAttribution: effulgentsia commentedOh, interesting. Which account should we set this to? Not every entity type has an author/owner, and we're not passing in $entity anyway.
Comment #51
moshe weitzman CreditAttribution: moshe weitzman commentedImplemented 1,3 from #48. I keps $entity->save() there just because it has caught errors a couple times which $entity->validate did not catch. In theory, we don't need to save but in practice it is helpful. I don't feel strongly either way.
#50: The file entity seems to save without any owner and I think thats fine for now. If we were to populate it, I imagine we would use $entity->user_id (or whatever its called). However, these generated entities don't yet have a user_id. We can add that but its going to be quite slow just like all entity reference population. Those do an entity_load_multiple() of every possible target entity AFAICT. The irony is that we only need the IDs in our case. Maybe we should bypass getReferenceableEntities and call buildEntityQuery() ourselves?
Comment #53
moshe weitzman CreditAttribution: moshe weitzman commentedForgot a use statement.
Comment #55
moshe weitzman CreditAttribution: moshe weitzman commentedRerolled. No other changes.
Comment #56
moshe weitzman CreditAttribution: moshe weitzman commentedWe are green here. Reviews welcome.
Comment #57
yched CreditAttribution: yched commentedNo more remarks on my side. @effulgentsia, feel free to RTBC :-)
Comment #58
effulgentsia CreditAttribution: effulgentsia commentedLooks great.
Comment #59
alexpottThis will generate possibly duplicate file names - I think this can be avoided by using
$random->name(10, TRUE)
instead. Creating a duplicate file name is problematic because this will generate an error.Comment #60
moshe weitzman CreditAttribution: moshe weitzman commentedFixed #59 and added telephone field support. Any other field types will happen in follow ups.
Comment #61
effulgentsia CreditAttribution: effulgentsia commentedComment #62
alexpottCommitted fe85f9b and pushed to 8.0.x. Thanks!
Fixed a few unneeded use statements which were being added and a minor code style nit.