If you enable search_api_database_defaults it depends on Core Search because the view modes search_index and search_display are part of Search Core.
We should "copy" those view modes and somehow make sure they can run in combination with drupal core search if someone wants to do so. I suggest searchapi_index and searchapi_display or something?
Estimated Value and Story Points
This issue was identified as a Beta Blocker for Drupal 8. We sat down and figured out the value proposition and amount of work (story points) for this issue.
Value and Story points are in the scale of fibonacci. Our minimum is 1, our maximum is 21. The higher, the more value or work a certain issue has.
Value : 13
Story Points: 2
Comment | File | Size | Author |
---|---|---|---|
#44 | 2624468-44--db_defaults_installation_ux--interdiff.txt | 10.98 KB | drunken monkey |
#44 | 2624468-44--db_defaults_installation_ux.patch | 15.02 KB | drunken monkey |
|
Comments
Comment #2
Nick_vhTaking this now - found the problem and testing the solution now.
Comment #3
Nick_vhThe solution is to add the configs to optional and only if you do not have the article and story type it will "silently" not install those configs. Core has support for this behavior and it is not a new pattern.
Comment #4
Nick_vhAdding a hook_requirements so that it would fail to install if the article and bundle are not available. Next up, adding the requirements for the specific fields that we need.
Comment #5
Nick_vhThis should be good for review.
Comment #6
borisson_++
This is not compliant with the other @file blocks.
any reason for the double quotes here?
Otherwise this looks great.
Comment #7
Nick_vhAdding newline and changing double quotes to single quotes
Comment #8
borisson_RTBC!
Comment #9
drunken monkeyThanks for solving this, looks good!
However, still a bit of nit-picking …
Comment #10
borisson_The change in the integration test is unrelated and I don't think it should go in with this patch. Otherwise the changes looks great.
Comment #11
drunken monkeyOK, thanks. Committed (split in two, so Order In The World is preserved).
Comment #13
Nick_vhSorry, a little follow up patch is required. And it does confuse me, the "links" parameter is shown in the export as a dependency
However, it is not present when looping over the fields. I'm going to say it is not required as everything installs just fine without it. The other change was just a mistake that I don't know how that can pass the test suite. I also installed it with the standard profile but the page content type didn't have field_image, ever.
Please review and commit this follow-up
Comment #14
Nick_vhComment #15
Nick_vhSo, this was more difficult than I thought. This adds a test for installing and uninstalling through the UI and make sure that it also removed the server and index. The test also removes the article content type and checks if the module fails to install. Since the configs are just exports, they are not being removed on uninstall. The reason is that they don't have a single dependency on config entities that makes them obsolete, and hence they are not marked for deletion. This is kind of strange as I even tried to make them a hard dependency on the module.
I had to move the view mode displays to optional and the view itself as well. Since we do check for the content types as the only hook_requirement, there is no need to add them to the install. It also conflicts if those view modes were already set up and we do not remove them on uninstall so they are fully optional. Since the view depends on those view mode displays, I had to move it to optional because the way the dependency system works. Eg, if you move that to "install" it fails to install because the stuff it depends on are listed in optional. Regardless of whether it adds it to your site or not, it will fail to install. Only the server and index are added to the install folder and those are also removed when uninstalling the module.
This brings me to a conflict, should we "uninstall" the configured view modes when uninstalling this module? Should we figure out if there are any other systems that use those view mode displays and if so, do not remove them?
Thanks
Comment #16
borisson_Do we need to check if these exist? They can't be uninstalled seperatly, can they? Not that it hurts, just wanted to know.
Either add a full stop after "Displays" or reflow the text to be one sentence.
Should those comments be there? I think they need to be near the relevant parts of the test. And They need a starting capital and period at the end.
80 cols
I think we should, but only if we can figure out that we're uninstalling them without breaking something else.
Comment #17
Nick_vhUpdated patch to be a bit more clear in what it does. I tried to calculate the dependent other configs but failed miserably.
Comment #18
Nick_vhThey can, they are all available in the UI.
Rerolled it to incorporate your comments
Comment #19
Nick_vhDoh, wrong way of getting the patch
Comment #22
borisson_Overall this looks great and I think this is probably the best we can do while staying sane.-
-
-
I have a couple of really small nitpicks, because I like nitpicking.
Capital/period.
The search api standards say not use short array syntax.
Too Many Capitals That Should Not Be Needed. ;)
Comment #23
Nick_vhaddressed those :)
Comment #24
Nick_vhComment #25
borisson_Awesome, let's get this in.
Comment #26
Nick_vhdrunken_monkey: Nick_vh: Huh. I actually would have said the server and index not being uninstalled with the module is a good thing.
[4:16pm] Nick_vh: then it should also move to optional
[4:16pm] drunken_monkey: That way, you can just uninstall the module again after installing it and it won't decrease performance completely unnecessarily.
[4:16pm] drunken_monkey: Also, I think it
[4:16pm] oraculi joined the chat room.
[4:16pm] drunken_monkey: 's more in line with the config philosophy of D8.
[4:16pm] drunken_monkey: Nick_vh: Why?
[4:17pm] Nick_vh: So installing a module adds stuff, uninstalling doesn’t remove anything?
[4:17pm] Nick_vh: doesn’t make a lot of sense
[4:18pm] Nick_vh: and then when you install it again, the server is in “install” and the module will fail to get enabled
[4:18pm] Nick_vh: so it has to be optional
[4:18pm] Nick_vh: imho
[4:19pm] drunken_monkey: Nick_vh: As said, I think it's in line with D8's config philosophy: the site owns the configuration, not the module.
[4:19pm] drunken_monkey: Maybe ask someone about it?
[4:20pm] drunken_monkey: If they wanted installed config to be removed along with the module, I think they'd coded it that way.
[4:20pm] Nick_vh: Well, if it depends on config entities, that’s how it works
[4:20pm] Nick_vh: but should the module fail to install after a second try then?
[4:20pm] drunken_monkey: And, as said, leaving the module enabled completely unnecessarily reduces performance.
[4:20pm] Nick_vh: I think that’s why it’s called optional
[4:20pm] Nick_vh: I’m fine in not removing it
[4:20pm] drunken_monkey: Hm, might be we should move it to optional, yes.
[4:21pm] drunken_monkey: Or add a check to hook_requirements() to give a more meaningful error message.
[4:21pm] drunken_monkey: (Unless the one you'd get currently is good enough already.)
[4:24pm] Nick_vh: well, that one is pretty clear currently
[4:24pm] Nick_vh: it checks all the required components
[4:25pm] Nick_vh: but it shouldn’t fail when re-enabling then
[4:25pm] Nick_vh: and I agree in not removing it, maybe we do need to add it to the description
[4:26pm] drunken_monkey: Like, "Can be uninstalled again right afterwards."? Sounds sensible to me, yes.
[4:26pm] dasjo left the chat room. (Remote host closed the connection)
[4:27pm] drunken_monkey: Failing on re-installing makes sense, too, I'd say, since that wouldn't do anything anyways. We should just have a good error message. "It looks like the default setup provided by this module already exists on your site. Installation failed." or something.
[4:27pm] dasjo joined the chat room.
[4:31pm] Nick_vh: the thing is that Core provides the error message if we have it in “install"
[4:31pm] Nick_vh: so sure, we can check if the resources are there in hook_requirements and then fail
[4:31pm] Nick_vh: but with a clear message
[4:31pm] Nick_vh: that would make sense to me
[4:31pm] drunken_monkey: OK, great!
[4:31pm] Nick_vh: I’ll c/p this conversation to the patch
[4:31pm] drunken_monkey: Then let's do it like that.
Comment #27
Nick_vhSo I think this patch complies with the discussion we had. You are now able to install the module and get all the configs, even if you already enabled the view display modes. On uninstall it will not remove the configs but when you try to install them again it will fail if you still have the index, server or view.
All of the above is also tested in an integration test. I have one exception which seems very unrelated so trying to test that here also.
Comment #28
Nick_vhremoval of a container rebuild line.
Comment #37
Nick_vhThis patch has to be committed, just for the fact it is 13.37kb!
Comment #44
drunken monkeyThanks, great job!
Just a bit of nit-picking, once again …
Also, the
partial_matches
change is a bit unfortunate, since it's of course completely unrelated, but if it needs to be there to pass the tests I guess it can't be helped.If no-one objects and the test bot is happy, too, I'll hopefully commit this in a few days.
In any case, thanks again for your work on this!
Comment #45
StryKaizerThis patch, which is in 8.x-1.x HEAD, added a dependency on 2 non-existant fields (field_image and field_links) on a CT page, for search_api_db_defaults
Installation is complaining about this and wont succeed.
Comment #46
borisson_The latest patch (#44) doesn't have the remark made in #45 anymore, I'd suggest we get that patch in so installation actually works.
Comment #47
drunken monkeyYes, definitely, thanks.
Committed.
Thanks again for your work, Nick!