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.
We need an example module for making Config Entities.
I'm working on something :)
Comment | File | Size | Author |
---|---|---|---|
#76 | interdiff.txt | 51.54 KB | Mile23 |
#76 | 2084301_76.patch | 33.75 KB | Mile23 |
#72 | interdiff.txt | 1.98 KB | Mile23 |
#72 | 2084301_72.patch | 33.34 KB | Mile23 |
#67 | interdiff.txt | 3.83 KB | Mile23 |
Comments
Comment #1
joachim CreditAttribution: joachim commentedHere's my work so far in case anyone wants to take a look :)
It's reasonably complete -- though I realize I've been working with the patch at #2084255: add label to rows in EntityListController on my system, so if that doesn't get in soon we'll have to implement an Entity List Controller here.
Comment #2
Mile23Looks like a lot of good stuff, thanks.
I think it might be useful to have an example of EntityListController, whether the base system provides a default one or not.
Needs tests, and a lot of todos in there. And a few things I noticed:
I'm not entirely sure how a config entity is supposed to work, but I'm inclined to assume this should be config_entity_example_robot_load() instead of just robot_load().
Should be @see
Comment #3
joachim CreditAttribution: joachim commented> + * http://previousnext.com.au/blog/understanding-drupal-8s-config-entities
> Should be @see
Not so much... that blog post is pretty out of date now -- I started off with the code from that and made a lot of changes before my module didn't crash! :)
Should it be a @see if it's just to give another author credit?
> I'm not entirely sure how a config entity is supposed to work, but I'm inclined to assume this should be config_entity_example_robot_load() instead of just robot_load().
You mean use '%config_entity_example_robot' as the menu placeholder? I'll look into it.
> I think it might be useful to have an example of EntityListController, whether the base system provides a default one or not.
Yup, that's a good idea.
> Needs tests,
I'll write some.
> and a lot of todos in there.
Some of those are things I just don't know...
Comment #4
Albert Volkman CreditAttribution: Albert Volkman commentedI'm not entirely familiar with the subject myself, but aren't upcasts now handled by ParamConverterInterface? Further, I believe if an entity is defined, it automatically gets upcasted via EntityConverter. I may be entirely wrong, of course.
Comment #5
marvil07 CreditAttribution: marvil07 commentedChanging status based on comments.
Comment #6
Mile23My mistake. Not @see but @link. https://drupal.org/coding-standards/docs#link
No, I mean functions in a module should start with the module name, like hooks. That way we avoid namespace collision. Again, I'm not sure how a config entity works, so it might need that function name.
Comment #7
socketwench CreditAttribution: socketwench commentedWasn't this hook removed? https://drupal.org/node/2184797
*.routing.yml can have title keys now.
Usually schemas are found in /config/schema
Why are we overriding this? I don't really see the point. The function name also changed to url().
Comment #8
socketwench CreditAttribution: socketwench commentedFixed a number of things. Still more left to fix.
Comment #10
joachim CreditAttribution: joachim commentedThanks! Here's some more work.
Some things that need attention:
- the default entity provided in the config file isn't showing up any more
- when I enable the module I get translation errors
Comment #11
socketwench CreditAttribution: socketwench commentedSelf assigning. Have lots of new changes in the queue. Just not ready for a patch. Hopefully tonight.
Comment #12
socketwench CreditAttribution: socketwench commentedChanges:
Still needs more comments to explain why. I can work on those.
Comment #13
tim.plunkettLooking good!
Fix this up
Can delete all of this now!
_entity_create_access: robot
Yes it does!
Should be _entity_access: robot.delete
Remove this :)
Docblocks!
No need for this
docblocks! Use {@inheritdoc} where possible
Contains \Drupal\...
Comment #14
Mile23As far as tim.plunkett's stuff: Awesome and thanks for the review.
Just a quibble on item #2: We don't need hook_menu(), but we do need hook_menu_link_defaults() to provide some in-site explanation of what's going on, and links to the new/view/edit/delete pages so newbies can find their way around.
Thanks to socketwrench and joachim for so much effort! :-)
Needs newline... Also in a few other places.
@see namespace\Robot
Also, at some point we have to explain what a config entity is, and how it's different from a non-config entity or whatever else. I think this explanation should be in the @defgroup block.
Thanks, PhpStorm! Now switch to @file. :-)
Even for properties where we have good defaults, we want to set them in annotation with documentation to explain what they are, and which ones can be safely omitted.
In the property docblock, explain the mapping in the annotation. Even as simple as "Maps to id for this entity through annotation." It might be illustrative to use a different name for the class property so the mapping is visible.
Comment #15
socketwench CreditAttribution: socketwench commentedFixed issues in #14, a few others I found, added lots and lots of comments.
Comment #16
socketwench CreditAttribution: socketwench commentedAdded @file docblocks.
Comment #17
Mile23It's shaping up.. Thanks.
I don't see Marvin in my entity list right off the bat. Is that the expected behavior?
What's supposed to be here?
I bet Drupal can't find this test. :-)
Also, this module needs tests...
Would super-duper love to see some more complex validation behavior, so people can learn how it works. Maybe some arbitrary string length or a numeric value that has to be validated.
Comment #18
joachim CreditAttribution: joachim commented> I don't see Marvin in my entity list right off the bat. Is that the expected behavior?
No, it's not. It's a bug which I don't know how to fix.
> +++ b/config_entity_example/config_entity_example.api.php
Nope, that should go.
> Would super-duper love to see some more complex validation behavior, so people can learn how it works. Maybe some arbitrary string length or a numeric value that has to be validated.
Is there anything we could do that is specific to config entities? If not, then isn't that best left to the Form API example? We could write a comment there pointing the reader to that.
Comment #19
Mile23OK, we do a @see form api or whatever example that ends up being.
Comment #20
BerdirNo need except you want to define the default hoosk (load, insert, update, delete, presave, ...) which are automatically called for each entity type, which would probably make sense...
Machine names for routes, local tasks and actions should be prefixed with "modulename.", instead of "modulename_".
No need for simplicity, you have an admin permission, use it...
Should have the full namespace.
Not necessary.
The config prefix is now optionally and forces the module name, so it would just be "robot", which is now the default, so you can leave it out.
That's very likely why the default content entity isn't found.
Remove this, works all based on the links/routes now.
Should use route_redirect with a Url object now.
There is no need for an access controller if you have an admin_permission. If you want to specify one as an example if access is not that trivial, you should explicitly document that this is only necessary when not defining an admin_permissions.
Nothing lax about it, this will use the specified admin_permission.
checking for view is a bit pointless as this entity doesn't define any view route/ability to display it, which I would leave out, block is the only special case that does use this.
also double ;;.
Suggestion: If you want to have an access controller as example, add a include some special thing like not allowing to default your default config entity (you can do so based on the id or by introducing a locked property similar to other config entities. And then describe that for everything else, you use the default implementation which will check admin_permission.
Comment #21
Mile23Basically every entity API related thing Berdir said just above would be great to include in docblocks and inline comments. :-)
Comment #22
socketwench CreditAttribution: socketwench commentedFixed the issues in #20.
I left the access controller in for now since I'm unsure if we should keep it or not. If we do keep it, what should it check?
Comment #23
socketwench CreditAttribution: socketwench commentedInterdiff. Hopefully I did that right.
Comment #24
jibran@param desc missing.
Missing blank line before @return and desc.
Why not $entity->label();?
Not needed.
desc missing.
more then 80 chars.
Use full namespace.
missing @param block
Shouldn't it be static::ALLOWED?
Comment #25
tim.plunkettAddressing #3, because that's what all list controllers do, shortcut for checkPlain
Comment #26
socketwench CreditAttribution: socketwench commentedAs for #9: The base class seems to only specify TRUE or FALSE. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
Fixed all other issues except #3. Still needs tests...
Comment #27
jibranThanks for fixing the issues. I think only @todo and tests are remaining and missing so tagging accordingly. Here is some more minor issues and suggestions.
:(
it is a todo but still 80 char limit applies ;)
block module has some docs.
Single line desc of function is missing. Then we can add explanation within 80 chars limit.
Builds
More then 80 chars.
lol
Comment #28
joachim CreditAttribution: joachim commented> + # TODO: explain by what magic '_entity_list' turns into using the entity list controller.
My most recent patch had that part rewritten:
@socketwench: could you check you got changes I made in that please?
Comment #29
BerdirYou removed hook_menu() now, which is correct, but you do want a hook_menu_link_defaults() for your menu link to the list page.
You need to look at the checkAccess() method, you're already relying on it or it wouldn't work: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
As mentioned above, I think a nice example would be to introduce a locked column and set that to true for your default config, then deny deletion in the UI for that. That's for example what contact.module does for the special personal category.
Comment #30
tim.plunkettNot sure why tests are needed here.
This is to show how to write a ConfigEntity, not how to write PHPUnit tests.
And it wouldn't be possible until #2134857: PHPUnit test the entity base classes is in anyway.
Comment #31
Mile23Needs tests because it's code and it has to be maintainable.
The rules for Examples with testing is that we're demonstrating APIs, so the demonstration has to work. If you have classes that aren't presentational, those have to be unit tested. For instance, an entity class. :-) However, the entity class in this case is just some properties so no biggie.
Here's the module checklist: #2209627: [meta] Module Checklist for Examples
Don't need this in .info.yml file.
Give at least a one-liner comment describing this local action and why it exists.
Needs @defgroup, which is where there should be a large chunk of documentation which describes the API we're showing off, why you'd want to use it, why you wouldn't want to use it, and where to look for more information.
Examples is documentation, written in docblocks, parsed by API module in api.drupal.org.
Here's an example of the kind of things we need: https://api.drupal.org/api/examples/field_permission_example%21field_per...
@ingroup config_entity_example
@link
Describe what the entity models, why it has the properties it has, what the expectations for it are, why it's a config entity and not something else.
Explain what the various properties do, what they map to, why they exist.
@ingroup config_entity_example
@ingroup config_entity_example
@ingroup config_entity_example
@ingroup config_entity_example
@ingroup config_entity_example
Comment #32
BerdirI agree with needing tests but I think unit tests are useless here. We're not interested in testing the implementation of those classes, what we want to test is the integration with core, and making sure that it still works when core changes. So what we need is a functional test that verifies they can be created and listed in the UI. And maybe a DUBT test with basic CRUD stuff like create, save, update.
Comment #33
Mile23Berdir: That's why I didn't add back the phpunit tag.
See the Examples module checklist: #2209627: [meta] Module Checklist for Examples
Comment #34
Mile23All the changes above, with some coding standards stuff.
Still needs functional tests, expanded newbie documentation, hook_menu_default_links().
Also: Is there a way to specify a blob of text at top of an entity list controller? I want to put "This is a list of the entity we created.. You can do the following with it..." Does ConfigEntityListController have a method I can't find, or a special trick? Or does it need to be hook_help()?
Comment #35
tim.plunkettComment #36
Mile23Thanks, tim.plunkett.
Added some descriptive text to the entity form.
Comment #37
BerdirSee below, only "create new thing" should be a local action, so this should just say that it adds a local action to create a new robot to the list overview or so.
This is not action. If anything, then list/edit/delete should be local tasks.
Comment #38
socketwench CreditAttribution: socketwench commentedI started working on the group documentation when I noticed there were other updates. Here's what I got so far:
Comment #39
Mile23Sorry, socketwench. Please reassign yourself if you'd like.
Comment #40
Mile23Comment #41
socketwench CreditAttribution: socketwench commentedRemerged @joachim's changes. Added several more comments.
Comment #42
jibranAwesome we have know a lot of documentation. Thank you @Mile23 and @socketwench.
Adds
white space.
I think intention here is to keep it to 80 char so this more then 80 chars.
Highlighting all the remaining @todo just for a self reminder for next review.
Full stop missing.
Gathers
as a
Gets
Full stop missing.
More then 80 chars.
Comment #43
socketwench CreditAttribution: socketwench commentedFixes as per #42, went through and completed TODOs.
Comment #45
jibranThanks for the fixes. Great work fixing all the @todos.
I fine with aa :D
Comment #46
Mile23Added hook_menu_link_defaults() and fixed #45.
Needs tests at this point, for the following stuff:
Anyone want to assign themselves? :-)
Comment #48
Mile2346: 2084301-46.examples.config-entity-wip.patch queued for re-testing.
Comment #49
Mile23OK. Here I go with the tests.
Comment #50
Mile23Added functional tests until I got an error that seemed very very strange, and probably has nothing to do with this project....
call_user_func() expects parameter 1 to be a valid callback, function 'contact_category_load' not found or invalid function name
Which is very strange because we're not doing anything with the contact module.
Also some adjustments for https://drupal.org/node/2200867
This test will fail, but might pass later.
Comment #52
tim.plunkettSee DateFormatFormBase for a better example of how this should be done.
Comment #53
Mile23Thanks, tim.plunkett, but you'll have to be more specific... DateFormatFormBase doesn't implement buildForm().
Comment #54
tim.plunkettIn this patch, you copy/pasted contact_category_load. You should use a method on the class.
From DateFormatFormBase::form()
The important part is the 'exists' => array($this, 'exists'),
Also in DateFormatFormBase
Comment #55
Mile23Thanks, Tim.
Unassigning myself because I was just writing tests...
Anyone who wants to take it up can go ahead. I might get back to this tomorrow if I can.
Comment #56
socketwench CreditAttribution: socketwench commentedAdded exists code based on #54.
Comment #58
socketwench CreditAttribution: socketwench commentedFixed the error in the test. Posting here since I still haven't figured out why I can't run D8 tests locally...
Comment #60
socketwench CreditAttribution: socketwench commentedFixed the default entity yml due to https://drupal.org/node/2234799
Comment #62
socketwench CreditAttribution: socketwench commented60: 2084301_60.patch queued for re-testing.
Re-testing due to https://drupal.org/node/2262989
Comment #63
Mile23Needs the menu_links.yml file makeover...
Edit: Ug. Forgot to delete the old hook_menu().
Comment #64
joachim CreditAttribution: joachim commented> * - entity_keys: Specifies the class variable(s) in which unique keys are
* stored for this entity type.
Could this be phrased a bit more clearly? At the moment I don't understand it at all.
- I think for something that's visibly a list, using '(s)' is unnecessary. It's usually always the case that you can have a list of one thing, so we don't need to say that.
- 'class variable(s)' - is this what used to be called 'entity properties'?
- 'unique keys' - is the label unique? It's a bit confusing.
What entity_keys seems to do is tell the entity system which object property to look in for standard properties it wants to find on an entity. So "label" = "wibble" would mean the entity system would look in $robot->wibble for the label. Maybe we should deliberately change these so it's easier to see how they work?
Comment #65
joachim CreditAttribution: joachim commentedIs the string 'robot' what ties these together?
When the system reads the file config/config_entity_example.robot.marvin.yml, how does it know to use Robot.php for the resulting object?
Comment #66
tim.plunkett#65, yes that's what that is.
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Config%21...
that's the function that finds out what entity type to use, and the entity type definition says which class has this ID in it. That part is just standard plugin behavior (mapping an ID to a class).
Comment #67
Mile23How does that look? Check out the interdiff to see what I did.
Comment #68
joachim CreditAttribution: joachim commentedCould we give a link to documentation about the format of this file, and what the different types are? Though all I've found is https://drupal.org/node/1905070, and I find it utterly impenetrable :/
Comment #69
joachim CreditAttribution: joachim commentedAlso, some questions I have that could do to be answered in the comments:
- how do you make Drupal notice a new entity type?
- how do you make Drupal notice a new default config entity file, or changes to it?
Comment #70
BerdirNeither of those really belong here I think? Entity types are discovered like everything else, by clearing the cache. And default config entities are like all default config, when the module is installed.
Comment #71
joachim CreditAttribution: joachim commentedOkay so links to docs to find that out? Because I've no idea where those are.
Comment #72
Mile23Added some info to the yml files to try and explain some of the things joachim mentioned.
Comment #73
joachim CreditAttribution: joachim commentedCould we add a custom property to the Robot entity, say, 'floopiness'?
I'm trying to do that in my own entity and the values aren't getting picked up from default config.
Comment #74
Mile2372: 2084301_72.patch queued for re-testing.
Comment #76
Mile23Re-roll for PSR-4, updated getCancelRoute() for https://drupal.org/node/2189619, added Floopy flag with test. :-)
Comment #78
Mile23Committed! http://cgit.drupalcode.org/examples/commit/?h=8.x-1.x&id=03d7d89a6f46ea5...
Make a new issue for new stuff.
Thanks, joachim, socketwench, and everyone else!