Part of meta issue #1802750: [Meta] Convert configurable data to ConfigEntity system
Converting the contact form settings to the config system would help the Drupal 8 Multilingual Initiative to have a focused, simple but prominent enough example implementation to work with to add internationalization support *afterwards*. Currently there are no subsystems converted that would need internationalization support naturally and I did not find ongoing issues for any of that area either, so let's start with this one to benefit D8MI's discussion on internationalization.
The implementation can take great cues from the image effects conversion that already landed and #1493108: Convert logging and error settings to configuration system. Documentation on API for conversion: http://groups.drupal.org/node/191283
Related issues
Follow-ups
#1816540: Convert both contact forms to FormControllers
Comment | File | Size | Author |
---|---|---|---|
#185 | 1588422-contact-config-185.patch | 16.67 KB | andypost |
#185 | 1588422-interdiff-185.txt | 951 bytes | andypost |
#184 | drupal8.contact-config.184.patch | 16.4 KB | sun |
#167 | contact_delete.png | 18.75 KB | andypost |
#163 | drupal8.contact-config.163.patch | 17.2 KB | sun |
Comments
Comment #1
andypostThe only thing that stops this issue is ability of new config system to enumerate sub-values. Example of this could be found at #1479466: Convert IP address blocking to config system
Once you going to get list of contact categories to put them into select...
Comment #2
sunCan you clarify what you mean by "enumerate sub-values", please?
The notion of nested keys is already built-in. If "categories.foo" and "categories.bar" are both sub-keys of "categories", then $config()->get('categories') will return an associative array holding the sub-keys foo and bar.
Comment #3
Gábor HojtsyWell, the image module code at least uses these two code snippets to enumerate all items saved or load a specific item respectively:
To me this looks like properly enough for what contact module needs (AND is an existing pattern used). Of course if there is going to be a better way, both image module and this patch could use it, but I don't see there is anything lacking to get started.
Comment #4
sunWhen talking about enumerating separate configuration objects (instead of sub-keys within a single config object), then #1552396: Convert vocabularies into configuration will add a range of higher-level API functions for that.
Comment #5
Gábor HojtsySo that looks like would convert the above to config_load_all('image.style', 'image_style_load'). It does not seem to change keys or storage, just the surface API, so we could move there in this issue too later if it gets in soon. Looks like that would not make this postponed.
Comment #6
dawehnerFor this conversion it seems to make sense to add a machine name to categories and use that internally.
Comment #7
dawehnerHere is a general description what i think should be done
Here is just a short start, 50% of the test classes are running, though there are quite a lot of errors remaining.
Comment #8
dawehnerDidn't saw the last comment of gabor, use his suggested function.
Comment #9
andypostMostly looks good
some debug dsm() are left here
same
Suppose we should drop useless table in hook_update_8000
dsm
Comment #10
dawehnerSome work to get the amount of test failures down. The only missing is that deleting a category doesn't work.
Comment #12
andypostthis should be done in hook_install() to fill actual the site admin email
Also this require sorting, probably we need a
_contact_get_categories()
because same code used in tests.use contact_load($name) no need to have the same function
not needed
site_mail
should be populated so how to deal with defaults???config should care about translation, see #1448330: [META] Discuss internationalization of configuration
Let's move this to one function
_contact_get_categories()
as mentioned above3rd fetching of categories
_contact_get_categories()
Comment #13
andypostAll variables are converted.
Introduced contact_categories() to get all categories
Fixed all tests
Added upgrade path
There's 2 @todo - copied from image module: about caching and str_replace for config prefix
Comment #14
Gábor Hojtsy@andypost: re config should care about translation, see #1448330: [META] Discuss internationalization of configuration, I've opened this one specifically because it is a great example to prototype language support on *once* the initial conversion landed in core. It does not care about language now either (minus the translatable tags on queries which does not really help certain aspects of it). So let's get the initial port done and committed first :) Thanks!
Comment #15
andypost@Gabor I got IRC talk with @sun about config and he planing to postpone this issue until commit of #1552396: Convert vocabularies into configuration because we have no common approach for converting table-based "content-related-staff" to config.
By the same time initial patch works and we can start experimenting with localization on this to find all troubles that we can't foreseen in theory.
In the current state I got troubles with configAPI by getting a list of objects
contact_categories()
and loading a config obejctcontact_load()
@sun mentioned about new function
config_load_all()
in #1552396-8: Convert vocabularies into configuration and which accepts a loader function (as I understand) and proposal to move to a config entity storage backend see #1552396-12: Convert vocabularies into configurationI'm not postponing the issue for further reviews, probably we could convert this to some kind of a light config-entity but it looks like overkill. OTOH we really need some kind of meta-data to convert schema-translatable bits to config flags to mark a key in config as translatable.
First place to apply for language
Most pain-points here. There's no static cache because it's now configuration (same in image module)
Another issue with $config_name that needs special processing.
Also this function loads a whole config objects in memory and passes out. But for most of contact forms I just need a keyed array name=>category(localized)
Comment #16
Gábor Hojtsy@andypost:
For multilingual support, I don't think it makes sense to work against a temporary patch that is going to be reworked in storage strategy / API and is being postponed for another issue. I hoped that going through this one fast can help set up a real ground for discussing multilingual support for config, since otherwise the discussion stalled. We can postpone this and practically postpone the multilingual discussion in parallel then.
Yeah, I explicitly don't want to mix in language support here. I wanted to have this done in a straight port. There is no language support for contact forms in core, except the markers for translatable in schema/queries, which we can live without for a month or two I think in the dev branch, while we figure out how do we want to do it. If we postpone the whole conversion on figuring this out, we discuss too many issues at once.
CMI has been going for 14 months, and we have 6 months until code freeze. Need to get pragmatic or accept losses.
Comment #17
andypostThis patch could be re-titled to introduce a machine names for contact categories anyway.
Probably we need review of @daveraid
@dawehner any reason to disallow changing of machine names?
The #disabled seems unnecessary here
Comment #18
Gábor HojtsyThere you go.
Comment #19
dawehnerAbout the renaming of machine names, this is common on some of the "exportables" in core (filter, menu, fields, but not all of them) that you can't rename them. The reason is that other things might rely on this machine_name to be constant.
For example this improves the deploy situation a lot. I agree though that for contact categories, i can't easily find a use case beside the deploy one.
Comment #20
Gábor HojtsyCMI switched to yaml in the meantime, not XML anymore as in the patch. I think it would make sense to do this port as-is now and then revisit later if we want to introduce an entity API like solution for this. To me that sounds pretty convoluted at least. But having this simple conversion in would let us experiment with language support on a concerete converted subsystem, which we don't have any examples yet that would need language support.
Comment #21
andypostYAML it is, all patches needs re-roll...
@Gabor do you think we should leave a todos in patch to mark translation points?
Comment #22
andypostProbably category.default should be a "state" not the config as @catch mentioned in #1591412-11: Convert tracker settings to configuration system
Comment #23
dawehnerI would think that the default category is configuration, it's a setting which you want to deploy from dev to live systems.
Wouldn't it be possible to use the category name lowercase and replace of spaces. I guess that's a similar problem to the migration
of filters in d7, there also just ids were used.
This patch just converts to yml.
Comment #24
andypostwe could only use transliteration module since we have no multilingual machine-name generator at all
@dawehner if you need to migrate a category & its "defaultness" so it make sense to have them in one config bundle, otherwise we get troubles with merging config values
Comment #25
Gábor HojtsyAs with all places where we need to introduce machine names, we can provide a default implementations and let people override/extend that if they need to. I don't think this should be a show-stopped for introducing machine names.
Comment #26
vasi1186 CreditAttribution: vasi1186 commentedRerolled the patch to the latest version of d8, without any other changes.
From my point of view, the patch seems pretty ok, it does replace the information from the contact table in the new config system, and the tests should also all pass now.
Comment #28
vasi1186 CreditAttribution: vasi1186 commentedLet's try again...
Comment #29
vasi1186 CreditAttribution: vasi1186 commented#28: 1588422-contact-config-28.patch queued for re-testing.
Comment #31
vasi1186 CreditAttribution: vasi1186 commented#28: 1588422-contact-config-28.patch queued for re-testing.
Comment #33
vasi1186 CreditAttribution: vasi1186 commentedRerolled the patch to the latest 8.x version.
Comment #34
Gábor HojtsyWhat is still holding this up? Any complaints against the latest patch?
Comment #35
gddI suspect we are going to resolve #1599554: Tutorial/guidelines for how to convert variables into configuration in the next 24 hours, and that will require a reroll of this, so you might want to hold off until that happens.
Comment #36
Gábor Hojtsy@heyrocker: Now four times your estimated time passed. There has not been any comments on #1599554: Tutorial/guidelines for how to convert variables into configuration for 23 days. Are you advocating postponing this issue on that indefinitely?
Comment #37
gdd#1599554: Tutorial/guidelines for how to convert variables into configuration is fixed, so this needs a reroll.
Comment #38
andypostIf machine name has no trim() on value so this require $name = trim($form_state['values']['name']);
same here
according #1599554: Tutorial/guidelines for how to convert variables into configuration names should be changed without contact_ prefix
Comment #39
andypostFixed #38
Comment #40
Gábor HojtsyAnything else left here to do?
Comment #41
andypostSuppose it's ready, but still needs review :)
Comment #42
sunFixing tag. Wasn't able to find this issue.
eh? This saves an empty array, if no default language was configured. It is impossible that this is what you want.
These should ideally be "id" and "label".
I'm currently trying very very hard to find and introduce a definite solution for configurable thingies over in #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) — hopefully, we'll be able to resolve that in the next few days.
In general, however, I guess you could already start to copy most of the basic concepts from there, especially for the administrative module implementation code, if you'd want.
It looks like these YAML files have been hand-coded, since string values are not quoted in YAML. Only integers, floats, and anything else that's not a string is quoted.
1) These settings could really use some more self-descriptive names. It's not clear what "default_status" relates to.
2) The flood threshold settings form a sub-set, so we should use sub-keys; e.g.:
1) Needs to be converted to config('system.site')->get('mail') now.
2) I'm not sure whether hook_install() isn't invoked too early during initial installation of Drupal. The site e-mail address gets only configured in the last screen of the installer; modules are installed upfront.
...which makes this detail a veeeery interesting use-case ;)
Didn't think about this yet.
As we haven't converted any configurable thingies yet, there hasn't been a discussion on whether the former database tables should be retained during the upgrade.
Contributed modules that might integrate with Contact module will be disabled during the Drupal core major version upgrade, and only enabled and upgraded at a later point in time.
Configuration objects should only be instantiated once within a local scope/context.
Comment #43
Gábor HojtsyTwo notes on the more complex questions:
- For the installation question, contact module could react to the installation being done and update its config?
- For removing/not removing the table, contrib module updates always worked with updated cores, it would just be the same here. Don't think we are doing anything new here.
Comment #44
Gábor HojtsyI clearly do not have people in D8MI to do "our own" issues even, so moving this off our sprint.
Comment #45
dawehnerI reverted these changes, because they are out of scope here.
Changed the naming all over the module.
What about "user_contact_default_status"?
This worked before, so i guess that's fine.
Let's keep the table for now.
Comment #47
alexpottThe patch for #1496534: Convert account settings to configuration system needs to convert the contact_default_status variable. I guess that one of these patches needs to be postponed until the other gets in.
Comment #48
dawehnerTo be honest we should have converted first the settings and on another issue the actual categories as this patch changes so much at the moment.
Comment #49
alexpottRerolled patch to apply cleanly on 8.x and removed variables in light of #48, #1496534: Convert account settings to configuration system and #1704530: Convert contact module variables to configuration system.
Couldn't create an interdiff for some reason - I think it's because #45 no longer applied cleanly.
Comment #51
alexpottFixed the broken upgrade path.
Comment #52
alexpottWhilst reviewing #51 I noticed the comment
// Migrate and drop {contact} table
incontact_update_8000()
however thedb_drop_table()
was removed due to comments by @sun in #42.Are we sure that this is the right thing to do? Wouldn't an 8.x version of a module that has been disabled during upgrade use CMI and not the database - and if it does isn't it doing something wrong?
Personally I think that once the schema is removed from contact.install and the table converted to config then the table should be removed because everything that is interacting with contact categories from then on should be doing it through CMI alone.
Comment #53
andypostAny reason to break all translations?
EDIT related issue #183678: Select Category via URL in Contact Form
Comment #54
sunRe-rolled against HEAD. This patch only serves as a "fallback".
Comment #56
sunAwesomeness.
First patch also contains the changes from #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)
Second patch only contains the changes relevant to Contact module, but won't pass without the former changes, so using .txt suffix.
Comment #58
larowlanAttached patch updates Module api test to include config in list of modules.
Interdiff from patch at #56 attached.
Comment #59
Dave ReidSomething I've noticed, is please do not use the entity URI as an admin page. Entity URIs should be the publicly-visible pages, which does not apply to contact categories, unless the URI is the contact form set to that category.
Comment #60
disasm CreditAttribution: disasm commentedreroll
Comment #61
disasm CreditAttribution: disasm commentedI removed the contact_uri function as well as the uri_callback lines from both hook_entity_info to satisfy what Dave Reid commented on.
Comment #63
disasm CreditAttribution: disasm commentednote: the patch larowlan made was created off of sun's patch containing [1668820]. I'm going to take sun's patch without the merge, and apply larowlan's interdiff to get this patch back to being a single one after I finish the patch review I'm currently doing.
Comment #64
disasm CreditAttribution: disasm commentedhere's a reroll of 58 based on the correct patch from #56. It will fail testbot because it's missing the stuff dependant on #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc).
Comment #65
disasm CreditAttribution: disasm commentedattached is my patch from #61 added back in. As well as a patch including #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) for the testbot.
Comment #67
Gábor Hojtsy#1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) is now committed, so this will need a reroll.
Comment #68
disasm CreditAttribution: disasm commentedattached is a reroll. Seeing errors in the last failed test about undefined index config prefix, so this will probably fail testbot.
Comment #69
disasm CreditAttribution: disasm commentedComment #71
disasm CreditAttribution: disasm commentedhere's a patch adding 'config prefix' to hook_entity_info and fixing entity_get_multiple calls with FALSE as 2nd param.
Comment #72
disasm CreditAttribution: disasm commentedComment #73
kbasarab CreditAttribution: kbasarab commentedI was under the impression all YML values had to be strings in quotes. Did I miss this change?
Why not make this less Ugly now? Only thing I can think is combining foreach loops into one but not sure if that is possible or not. Its definitely not the ugliest I've seen. Could maybe do without the @todo.
Comment #74
tim.plunkettSee #1668820-106: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc), part of that is being converted to a helper
Comment #75
aspilicious CreditAttribution: aspilicious commentedStrings don't have to be in quotes, but integers should be. Yeah yeah...
are all strings when importing the .yml file.
Look at the core config files to see how it's used.
Comment #76
cosmicdreams CreditAttribution: cosmicdreams commentedSo ultimately this patch will be postponed until #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) is committed, no?
Comment #77
Gábor Hojtsy@cosmicdreams: there are just some followup cleanup happening there, the infrastructure needed for conversions is in place.
Comment #78
sunWe definitely don't need to wait for #1760358: Provide a way to extract the ID from a config object name here.
Updated for config prefix declaration moved into hook_entity_info().
Comment #79
cosmicdreams CreditAttribution: cosmicdreams commentedWow, I've never seen list() used that way, but i suppose that since the patch went green that's legal. But I'm curious why that's better taking the third item of the array that explode produces.
I find this kind of use for list to be confusing. My knee-jerk reaction was, "What's two purpose-less commas here for?"
Comment #80
Robin Millette CreditAttribution: Robin Millette commentedI use list() = explode() all the time. A comment would help, something explicit like
Comment #81
sunThe exact details for how the ID/machine-name extraction is going to work and look like in the future will be figured out in #1760358: Provide a way to extract the ID from a config object name
I will work on simplifying that, but my battle plan is to see more more actual use-cases and code in core first, before drawing conclusions on the best way to combat the problem. It is perfectly possible that we're going to change the entire design of the config import callbacks. Let's please not hold up this patch on that.
This patch looks RTBC to me — but I've authored very large parts of it, so I'm not eligible to mark it as such.
Comment #82
aspilicious CreditAttribution: aspilicious commentedI would like to have an upgrade path test for this.
Especially for this part:
Don't want the upgrade path to break because someone chose a funky name as contact category.
Chinese people for example use different characters.
And I would love to have the test to go in with this patch (and not in a followup).
Comment #83
Dave ReidYeah I have a feeling we need to migrate the IDs of these categories based on something like 'category_' . {contact}.cid. This should have tests for the upgrade path included.
Comment #84
sunUgh. Upgrade path tests are a PITA to write and will surely delay this issue for many weeks. :(
That said, I've changed the module update and applied the standard pattern for everything we're migrating from serial IDs to machine names: Taking over the serial ID as-is as machine name.
I hope that resolves the major concerns with regard to the upgrade path.
Furthermore:
Renamed ConfigurableBase to ConfigEntityBase.
Removed config module dependency by making config module required.
Ensure unique machine names for converted contact categories by taking over serial ID directly.
Comment #86
sunFixed bogus dependency resolution for config module.
The remaining part here should be the upgrade path tests... I'm seriously unhappy that we're blocking this issue on that, although I do understand that we need some tests for that, but I actually do not see us writing individual upgrade path tests for every single Configurable that is being converted into configuration. Instead, I think we want to write a single "catch-all" upgrade path test for all possible configurables in Drupal core to verify the raw data conversions from database tables into config objects/files.
Comment #87
dagmar#86: config.contact.86.patch queued for re-testing.
Comment #89
sunMerged in HEAD.
Comment #90
xjmLove it! Reading this patch just reinforces for me what an architectural win configurables are. I haven't reviewed the upgrade path or the test coverage yet, but all the logic looks great.
Note that I disagree with #86--I think that thorough upgrade path test coverage for configurables is essential, since no two are actually stored the same way at present. Given that CMI is radically new from the ground up, it's a huge deal to make sure there aren't unexpected oddities while switching to the new storage. Generic tests also sound like a great idea, but I think module-specific ones are also important.
Tagging for the upgrade path tests so potential contributors can find the issue.
Comment #91
plachAren't we using entity form controllers here?
Comment #92
andypostI think we need a common test which should be extended with each converted variable to cmi. Probably there's should be one already.
EDIT @plach is right, this module should be the example of ConfigEntity with Form and multilingual
Comment #93
plachI'm not sure whether we will be able to exploit entity forms to translate configuration (I still have to catch-up with the latest progress made by Jose) but I guess we want them anyway. Translatability is certailny not the only benefit involved.
Comment #94
andypostIntroduced a default entity form controller
changed a path to delete confirmation to more shorter
Comment #95
andypostFixed broken tests and copy/paste errors
Comment #96
plachLooks good to me: what about calling it
ContactCategoryFormController
which makes more clear which is the involved entity?Edit: Better, since we are in the Contact namespace simply
CategoryFormController
.Just one question: why are we saving parts of the object in the entity-building phase (before the entity is actually built!) and not in the save submit handler?
Comment #97
yched CreditAttribution: yched commentedAt least some of those are defined in ConfigEntityBase, right ?
If so, they shouldn't appear here as well.
Not too familiar with EntityFormController yet, but isn't it odd to have i/o and redirect stuff inside a save() method ?
7 days to next Drupal core point release.
Comment #98
plach@yched:
It's the submit handler for the save action, it has to deal with redirects :)
Comment #99
yched CreditAttribution: yched commentedAlso, that's really not specific to this issue, but my eyes bleed at
entity_create('contact_category')
.I wished I'd never have to write entity_load('field'), nor a hook_entity_info() entry for 'field_instance'.
I thought we had separated notions of config and content entities, but apparently this got washed away in the reshuffles since then ?
If all those entity types live in a flat space, and when 'field' becomes an entity type next to 'node', I fear the insane cross dependencies and race conditions on rebuilding entity_get_info()...
Comment #100
yched CreditAttribution: yched commented@plach: ok - then I guess save() tickles me as a slightly misleading name for a submit handler, but that's not for this patch either.
Comment #101
plachIMHO it feels very natural once you get used to it, however I agree this is not the right place to discuss it.
Comment #102
andypostActually most of all core entities are using pattern with delete confirm and all implementations are different, suppose we need a boolean in default controller to allow this functionality.
Contact or ContactCategory - we could rename it latter in follow-up after neverending bikeshedding... as always
This because 'selected' is not a part of entity and should not be in entity-array. OTOH submit could only care about redirect when delete pressed, and all other logic would be moved to save. I made it specially to leave only entity related core for save.
I think a patch is ready to be commited, but we need a bit more reviews, also I suppose here is enough tests for.
Also it needs re-roll after #1785974: Move ConfigEntity into a Core component
Comment #103
plachI don't want to start a bikeshed either, but aiming for consistency since the beginning is not bikeshedding: all the existing form controllers have the entity in their names, or at least the operation in the case of the profile form controller, for instance. I won't insist on this, however.
Comment #104
andypostQuick re-roll with rename to CategoryFormController
Also moved update of selected category to save() from submit()
Comment #105
andypostMerged head, there's a cleanup happens in #500866: [META] remove t() from assert message
Actual change is
EDIT: Needs re-roll because commited #1785974: Move ConfigEntity into a Core component
Comment #106
podarok#105: 1588422-contact-config-105.patch queued for re-testing.
Comment #108
andypostChasing head, fixed namespace after #1785974: Move ConfigEntity into a Core component
Comment #110
andypostWrongly posted interdiff (1588422-interdiff-105vs106.patch) with .patch extension
Comment #111
andypostAlso there's issue to change overview page #599770: Clean up the contact forms listing UI: Allow to set the default category and weights on the listing page
Comment #112
andypostNow this entity should be converted to plugin! For example of conversion #1763974-15: Convert entity type info into plugins
Comment #113
tim.plunkett#112, that can and should happen after this initial effort. At a minimum, it's blocked on the same issue the issue linked is.
Please proceed as usual, the plugin conversion will be simple after this gets in.
Comment #114
andypostSmall cleanup patch, no need to use id() and label() in controller
Comment #115
andypostAnd removed dependency on config module
Comment #117
andypostFixed broken test
Comment #118
cosmicdreams CreditAttribution: cosmicdreams commentedMostly this is RTBC, I have a quibble about the test to delete contacts. But this patch is green and it looks solid. It's also a great example for anyone else who is planning on using configurables.
haha, YES!
This is a bit of an indirect test. You're checking to see if you're getting the proper status message after a deletion. It would be better to:
1. prove that a category exists.
2. delete that category
3. prove that the category no longer exists.
Comment #119
andypostAdded check to make sure that category actually deleted
@cosmicdreams testSiteWideContact() depends on that categories are deleted so:
1. entity_load_multiple()
2. drupalPost()
3. see attached intediff
Comment #120
tim.plunkettWhat is the reasoning for these hooks?
Comment #121
andypost@tim.plunkett as docs says, we should for each config entity hook_config_import_delete
EDIT: Suppose we requires this to properly clean caches
Comment #122
Anonymous (not verified) CreditAttribution: Anonymous commentedthese hooks are necessary to get ConfigStorageController::save() called?
Comment #123
Anonymous (not verified) CreditAttribution: Anonymous commentedtalked to timplunkett about this in IRC and he was concerned that every module that just needs ConfigStorageController::save() would be implementing the same boilerplate. i guess we could make some helpers like this:
that wouldn't require any changes to config_import_invoke_owner(), but doesn't save us that much. i'm not sure how to eliminate the need for these hooks without completely changing config_import_invoke_owner().
Comment #124
tim.plunkettThe same bit of code from #120 is in most of the ConfigEntity issues that have cropped up, and will likely make it into all of them.
So, a rewrite of config_import_invoke_owner() to make that easier would be great, or a helper.
Possibly would be best served as a static method on ConfigStorageController?
Comment #125
andypostI think this out of scope this patch and should be done in follow-up. This issue already has 125 comments so I filed #1806178: Remove code duplication for hook_config_import_*() implementations of config entities
Comment #126
Gábor HojtsyI agree with @andypost, getting this in should also help provide plenty more testing data for the multilingual initiative so further cleanup / generalization work would be great to do in a followup. Any other concerns?
Comment #127
sunThe battle plan was to implement these hooks for a couple of more Configurables, and only afterwards check which parts of the implementations are actually identical, which parts are similar/equal-ish, and which parts need to remain in individual implementations. And yes, this should be discussed in #1806178: Remove code duplication for hook_config_import_*() implementations of config entities - thanks for creating it ahead of time :)
Comment #128
tim.plunkett+1 to that, sorry for partially derailing this issue :)
Comment #129
Gábor HojtsySo looks like since all concerns were resolved since @cosmicdreams' review and no other concerns standing, this should be RTBC then.
Comment #130
tim.plunkettThis circumvents EntityListController by loading and sorting entities directly, and then adding operations links, instead of providing its own subclass.
In addition, the URLs for these actions are in the form of $prefix/$op/%id, not $prefix/%id/$op, which is inconsistent with the rest of core.
For example, this is Field UI:
admin/structure/types
admin/structure/types/manage/%node_type/edit
admin/structure/types/manage/%node_type/fields
admin/structure/types/manage/%node_type/display
admin/structure/types/manage/%node_type/delete
Meaning that admin/structure/types/manage is the prefix
This has
admin/structure/contact
admin/structure/contact/manage/%contact (for edit)
admin/structure/contact/delete/%contact
The $op is before it, which is incompatible with the operations provided by the list controller.
I would propose
admin/structure/contact/manage/%contact/edit
admin/structure/contact/manage/%contact/delete
----
Much of this confusion is because we never had a proper change notice for ConfigEntity, so each implementation solved problems on its own. I'm going to work on that now.
Comment #131
xjmMeta issue for these: #1802750: [Meta] Convert configurable data to ConfigEntity system
Let's get the stuff about how to convert paths to the list controller documented there.
Comment #132
tim.plunkettAssigning to myself for a reroll.
Comment #133
tim.plunkettThis is what I meant.
Comment #134.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #135
andyposttests are broken because off changed admin urls.
This requires new testAdminOverview() testing method to ensure that categories are sorted by their weight
Comment #136
tim.plunkettI fixed the tests, and also extended the correct class, since ConfigEntityListController::load() calls ConfigEntityBase::sort() for you.
Until the category needs custom sorting other than weights, there's no need for a test.
Comment #137
andypostJust a few fixes, should be grean and RTBC
Comment #138
tim.plunkettI'm not sure that this is a proper usage of the list controller... Just because it happens to sort it.
This should be reverted.
Also, why wasn't contact_site_form() converted as well?
Comment #139
andypostSo reverted controller and made usage of id() all over formController
Also:
- changed message to be more descriptive about operation (added/updated)
- Changed a execution of parent form method to continue work on d8mi issues in follow-ups
There should be follow-up to make ConfigEntity support enforceIsNew to make UI messages more consistend
Comment #140
andypostEnough for today
Comment #141
sunThis is required to stay; common for all entity delete confirmation forms: The entity ID is put into form value that uses the key of the entity ID.
Comment #142
sunOne more thing:
For this patch and Contact module it might be acceptable, but for all other ConfigEntity/Configurable conversion issues, please do NOT perform the EntityListController and EntityFormController migrations in the same patch.
The conversion to configuration is complex enough. As in this patch, that conversion usually requires test changes already. The additional entity system changes are moving huge parts of the code into different locations, making the entire thing impossible to review. Due to the test changes required for the config conversion, that means we cannot trust the test results anymore.
So please do not merge the additional entity system changes into a config system conversion patch. The entity system changes can happily be follow-up issues to the config conversion. Mixing them is scope creep and detrimental for making progress.
Thanks!
Comment #143
andypostSo reverted back id key of confirm_form()
Comment #144
andypostRe-rolls after #1480854: Convert operation links to '#type' => 'operations'
Comment #145
andypostPatch and interdiff
Comment #146
podarok#145 looks very nice!
Comment #147
tim.plunkettIdeally the wrapper for entity_load would only exist for menu load arguments (see #1757586: Remove MODULE_load*() & Co functions in favor of entity_*() functions) but there is the 'exists' for machine_name too.
Still, the middle call should probably be entity_load()
I'd say this is very very close to RTBC.
Comment #148
andypostSo here's a change
Comment #149
andypost#148: 1588422-contact-config-148.patch queued for re-testing.
Comment #150
podarok#148 looks good for me
Looks like this very usefull for GTD #1777070: Refactor and clean up source string location handling here (Translating all the config)
making this major
Yet one review for RTBC
Comment #151
podarokgm
didn`t see #129
making this RTBC
Comment #152
tim.plunkettThis is not major, we have a major meta for them.
But, RTBC +1. Awesome work @andypost et al!
Comment #153
webchickAwesome!! It's SO great to have a D8MI-affecting patch in core, as well as a ConfigEntity! :D
Committed and pushed to 8.x.
Comment #154
andypostAs it was mentioned above about follow-up for contact forms #1816540: Convert both contact forms to FormControllers
Comment #154.0
andypostAdded related issues
Comment #155
sunHappy to see this in.
But I'm a bit surprised that this got committed. The identical state (or actually a better one) was RTBC two months ago already, but got blocked on missing upgrade path tests. Suddenly, this patch went in without. I surely won't complain though.
What I do complain about are various bogus changes in the committed patch. Fixes attached.
Comment #157
Lars Toomre CreditAttribution: Lars Toomre commented@sun I noticed in your patch in #155 that you are eliminating most assert messages in ContactSitewideTest.php. I am not sure what your reasoning for that is. Could you elaborate?
Comment #158
andypost@sun Please leave the issue fixed and file another follow-ups for:
- default_category + tests
- tests clean-up
- configEntity itself
This does not works for configurables...
Magically this was lost...
Let's fix it in follow-up
this clean-up needs special follow-up
this broken
Comment #159
xjmTest coverage is a gate; I'd agree with marking it Needs work on that. It's not "Test cleanups"; it's "upgrade path tests, period". I'm not going to play issue status tug-of-war, but if not, then that's a major or critical.
@sun's changes in #155 look correct to me, other than the
enforceIsNew()
which I do not know about, and of course the test failures.@Lars, for background on why the assertion messages are removed, see #1601146: Allow custom assertion messages using predefined placeholders and #1601202: Make simpletest assertions output both the default and custom message texts. Custom assertion messages that are more vague than the default for the assertion make debugging more difficult. I'm not as adamant on this point as @sun is but I have also had to waste time on unspecific assertion messages.
Comment #160
andypostThis issue has a lot of comments so I filed #1817182: Add upgrade path tests for contact category config conversion
Let's leave this fixed
Comment #161
webchickxjm is of course right. Sorry, I was a bit too hasty on this one. I want to keep the commit in so D8MI can make use of it, but marking back to needs work for test coverage (also tagging, so I don't miss that again :)). We can go ahead and incorporate sun's improvements here too.
That said, I'm not sure how good in general we have been doing with making sure these CMI conversions all have proper upgrade test coverage. We might need a major task to go back and clean that up.
Comment #162
webchickOops. Missed #160. Yeah 100+ comments in is a good point, so we can handle at #1817182: Add upgrade path tests for contact category config conversion I guess. Let's get a follow-up for sun's changes too.
Comment #163
sunThat issue is about upgrade path tests. The follow-up patch here is about fixing bogus code that was introduced in this issue.
Let's do ourselves a favor and just fix the mess.
re #158:
Attached patch gets us back to sanity.
Comment #164
amateescu CreditAttribution: amateescu commentedI think all the improvements from #163 are good, and 2 or 3 more comments in this issue won't hurt anyone :)
Comment #165
xjm#163 looks correct to me as well.
Comment #166
xjmWe should probably also add to the existing change notifications for each converted ConfigEntity type (similarly to what was done for the entity conversion in http://drupal.org/node/1400186):
http://drupal.org/node/1818734
http://drupal.org/node/1734556
http://drupal.org/node/1799642
Comment #167
andypostThis patch introduces wrong UI pattern so Delete is displayed as Tab when you edit any category
Why not move this to hook_menu()
This displays Delete tab which was deprecated in D7. DO we wanna to return this back?
Comment #168
xjmHmm, not sure about the UI change. @amateescu points out that taxonomy terms and comments have a delete tab.
We should probably use a separate title callback for the
PASS_THROUGH
rather thandrupal_set_title()
; that's a good point.Comment #169
andypostAs I see all over a core we have Delete tab only for entities that have View tab...
Also about enforceIsNew()
Comment #170
tim.plunkett#169 reflects my understanding of enforceIsNew(), and the above patch changes that, and I'm not sure what it means :)
Comment #171
sun1) The page title is not the menu link title. Entirely different can of worms. See ContentEntity menu router info and page callbacks for reference.
2) An entity's tab_root normally bundles view, edit, delete, and potentially other tasks. view == edit, the default local task in case of ConfigEntity. We should follow that pattern. And very potentially consider to drop the "Delete" button from the form instead - if anything is misplaced, then it's rather that destructive button in an otherwise constructive interface.
3) enforceIsNew() obviously works only for Configurables that do not have an ID yet. It does not work for Configurables that have an ID already, since a Configurable is singular, unique, and cannot be duplicated.
Comment #172
andypostThere was #183678: Select Category via URL in Contact Form that was commited but lost somehow. In this context tabs makes sense
Comment #173
fagoExactly - enforceIsNew() is not necessary for config, if it's not there yet it will be created with the given machine-name anyway I suppose. Thus, for config the method is unnecessary and can be left aside. Why bother with it?
Comment #174
andypostWe need a common approach for entity->save() to detect create/update for config Entities to write watchdog/UI messages
Comment #175
sunTemporarily putting this on hold. I'm working on #1798944: Convert config_test entity forms to EntityFormController and will introduce massive ConfigEntity CRUD tests there.
Comment #176
xjmWell, let's take care of adding to the change notification, at least.
Comment #177
andypostI've updated all related issues with links to this issue, suppose we could change title and priority
http://drupal.org/node/1818734
http://drupal.org/node/1734556
http://drupal.org/node/1799642
Comment #178
andypost@sun please re-roll #163, we got commited #1798944: Convert config_test entity forms to EntityFormController
Also I think we should remove critical priority because all change notices are updated #177
Comment #179
Gábor HojtsyThe updated change notices look relevant to this issue to me.
Comment #180
aspilicious CreditAttribution: aspilicious commentedDon't see any reason why this should be postponed?
Comment #181
Gábor Hojtsy@aspilicous: it was RTBC and set to postponed in #175, I just moved back into that state, but seems like the issue it was postponed on was committed. If the latest patch still looks good and passes, this should in fact be back to RTBC, not active, as per above discussion (or some patch review or needs work state at least).
Comment #182
Gábor Hojtsy#163: drupal8.contact-config.163.patch queued for re-testing.
Comment #184
sunRe-rolled against HEAD, and adjusted to new/correct practices.
Comment #185
andypost#184 is RTBC, reviewing it I found a minor changes at doc-blocks so same patch + 2 new hunks
Comment #186
Lars Toomre CreditAttribution: Lars Toomre commented@andypost - I think out new practice according to #1487760: [policy, no patch] Decide on documentation standards for namespaced items is to have all namespaced classes start with a fully qualified path. In other words, the class reference is suppose to start with a '\'.
Comment #187
andypost@Lars agree on that, but introducing this changes in this patch will make that one much bigger so let's make it in follow-up after #1487760 would be marked as fixed
Comment #189
andypost#185: 1588422-contact-config-185.patch queued for re-testing.
Comment #190
sunYes, this is still ready. Thanks for the phpDoc tweak, @andypost!
Comment #191
catchThe drupal_set_title() calls are irritating. I'd been thinking/ranting about drupal_set_title() at BADCamp so I've gone ahead and opened #1833342: Use #attached (or similar) for the page title.
Otherwise this looks great so committed/pushed to 8.x.
Will need a change notice.
Comment #192
Gábor HojtsyOpened #1834002: Configuration delete operations are all over the place now that this has both a Delete button and tab (like taxonomy terms) while other things don't work like that (they either have one or the other).
Comment #193
swentel CreditAttribution: swentel commentedThis issue is listed at #1802750: [Meta] Convert configurable data to ConfigEntity system which states once it's done the issue number should be added to the change notice at http://drupal.org/node/1818734 - which is the case, so I think this can be marked fixed then - feel free to correct me of course.
Comment #195
yched CreditAttribution: yched commentedUnless I'm mistaken, the config files created in the upgrade function do not have a uuid entry ?
Comment #195.0
yched CreditAttribution: yched commentedUpdated issue summary.