#193 | 2796173-field_layout-193.patch | 71.07 KB | tim.plunkett |
|
#193 | reorganize-tests.txt | 8.19 KB | tim.plunkett |
#193 | 2796173-field_layout-193-interdiff.txt | 9.07 KB | tim.plunkett |
#191 | 2796173-field_layout-191-interdiff.txt | 5.06 KB | tim.plunkett |
#191 | 2796173-field_layout-191.patch | 69.44 KB | tim.plunkett |
|
#190 | 2796173-field_layout-190-interdiff.txt | 3.63 KB | tim.plunkett |
#190 | 2796173-field_layout-190.patch | 68 KB | tim.plunkett |
|
#189 | 2796173-field_layout-189-interdiff.txt | 2.83 KB | tim.plunkett |
#189 | 2796173-field_layout-189.patch | 67.07 KB | tim.plunkett |
|
#187 | 2796173-field_layout-187.patch | 66.98 KB | tim.plunkett |
|
#187 | 2796173-field_layout-187-interdiff-from-127.txt | 34.96 KB | tim.plunkett |
#181 | 2796173-field_layout-181-interdiff.txt | 6.44 KB | tim.plunkett |
#181 | 2796173-field_layout-181-PASS.patch | 66.98 KB | tim.plunkett |
|
#181 | 2796173-field_layout-181-FAIL.patch | 67.23 KB | tim.plunkett |
|
#159 | 2796173-field_layout-159-interdiff.txt | 877 bytes | tim.plunkett |
#159 | 2796173-field_layout-159.patch | 66.95 KB | tim.plunkett |
|
#157 | 2796173-field_layout-157.patch | 66.96 KB | tim.plunkett |
|
#157 | 2796173-field_layout-157-interdiff.txt | 16.45 KB | tim.plunkett |
#154 | 2796173-field_layout-154-interdiff.txt | 3.47 KB | tim.plunkett |
#154 | 2796173-field_layout-154-PASS.patch | 67.34 KB | tim.plunkett |
|
#154 | 2796173-field_layout-154-FAIL.patch | 67.04 KB | tim.plunkett |
|
#150 | 2796173-field_layout-150.patch | 66.09 KB | tim.plunkett |
|
#150 | 2796173-field_layout-150-interdiff.txt | 15.55 KB | tim.plunkett |
#127 | 2796173-field_layout-127.patch | 65.35 KB | tim.plunkett |
|
#127 | 2796173-field_layout-127-interdiff.txt | 9.88 KB | tim.plunkett |
#116 | 2796173-field_layout-116.patch | 64.44 KB | tim.plunkett |
|
#116 | 2796173-field_layout-116-interdiff.txt | 1.25 KB | tim.plunkett |
#113 | assign_region.png | 115.43 KB | xjm |
#113 | select_layout.png | 116.82 KB | xjm |
#111 | 2796173-field_layout-111.patch | 64.43 KB | tim.plunkett |
|
#111 | 2796173-field_layout-111-interdiff.txt | 1.69 KB | tim.plunkett |
#111 | Screen Shot 2016-12-12 at 9.59.33 AM.png | 358.1 KB | tim.plunkett |
#111 | Screen Shot 2016-12-12 at 9.59.13 AM.png | 344.19 KB | tim.plunkett |
#111 | Screen Shot 2016-12-12 at 9.58.34 AM.png | 381.06 KB | tim.plunkett |
#111 | Screen Shot 2016-12-12 at 9.58.11 AM.png | 340.6 KB | tim.plunkett |
#105 | 2796173-field_layout-105.patch | 64.38 KB | tim.plunkett |
|
#105 | 2796173-field_layout-105-interdiff.txt | 10.02 KB | tim.plunkett |
#94 | screenshot-d8.dev 2016-12-10 03-17-41.png | 127.09 KB | jibran |
#94 | screenshot-d8.dev 2016-12-10 03-17-21.png | 438.15 KB | jibran |
#93 | 2796173-field_layout-93.patch | 64.44 KB | tim.plunkett |
|
#92 | 2796173-field_layout-92-interdiff.txt | 12.1 KB | tim.plunkett |
#92 | 2796173-field_layout-92-do-not-test.patch | 64.44 KB | tim.plunkett |
|
#92 | 2796173-field_layout-92-combined.patch | 129.48 KB | tim.plunkett |
|
#87 | 2796173-field_layout-87-interdiff.txt | 6.74 KB | tim.plunkett |
#87 | 2796173-field_layout-87-combined.patch | 119.24 KB | tim.plunkett |
|
#87 | 2796173-field_layout-87-do-not-test.patch | 55.93 KB | tim.plunkett |
|
#84 | 2796173-field_layout-84-combined.patch | 119.32 KB | tim.plunkett |
|
#84 | 2796173-field_layout-84-do-not-test.patch | 56.01 KB | tim.plunkett |
|
#81 | 2796173-field_layout-81-do-not-test.patch | 55.32 KB | tim.plunkett |
|
#81 | 2796173-field_layout-81-combined.patch | 198.16 KB | tim.plunkett |
|
#78 | 2796173-field_layout-78-interdiff.txt | 17.49 KB | tim.plunkett |
#78 | 2796173-field_layout-78-combined.patch | 190.14 KB | tim.plunkett |
|
#78 | 2796173-field_layout-78-do-not-test.patch | 55.18 KB | tim.plunkett |
|
#74 | 2796173-field_layout-74.patch | 79.53 KB | tim.plunkett |
|
#74 | 2796173-field_layout-74-interdiff.txt | 1.95 KB | tim.plunkett |
#72 | 2796173-field_layout-72.patch | 66.61 KB | tim.plunkett |
|
#72 | 2796173-field_layout-72-interdiff.txt | 31.69 KB | tim.plunkett |
#71 | 2796173-field_layout-71-interdiff.txt | 24.81 KB | tim.plunkett |
#71 | 2796173-field_layout-71.patch | 58.38 KB | tim.plunkett |
|
#70 | 2796173-field_layout-70.patch | 57.58 KB | tim.plunkett |
|
#70 | 2796173-field_layout-70-interdiff.txt | 1.42 KB | tim.plunkett |
#68 | 2796173-field_layout-68.patch | 57.58 KB | tim.plunkett |
|
#68 | 2796173-field_layout-68-interdiff.txt | 17.59 KB | tim.plunkett |
#66 | 2796173-field_layout-66.patch | 47.26 KB | tim.plunkett |
|
#66 | 2796173-field_layout-66-interdiff.txt | 3.72 KB | tim.plunkett |
#64 | interdiff-2796173-61-64.txt | 435 bytes | samuel.mortenson |
#64 | 2796173-field_layout-64.patch | 46.75 KB | samuel.mortenson |
|
#61 | 2796173-field_layout-61.patch | 46.62 KB | tim.plunkett |
|
#61 | 2796173-field_layout-61-interdiff.txt | 8.81 KB | tim.plunkett |
#57 | 2796173-field_layout-57.patch | 46.74 KB | tim.plunkett |
|
#57 | 2796173-field_layout-57-interdiff.txt | 5.35 KB | tim.plunkett |
#54 | 2796173-field_layout-54-interdiff.txt | 1.63 KB | tim.plunkett |
#54 | 2796173-field_layout-54.patch | 45.39 KB | tim.plunkett |
|
#52 | 2796173-field_layout-52.patch | 45.23 KB | tim.plunkett |
|
#52 | 2796173-field_layout-52-interdiff.txt | 4.79 KB | tim.plunkett |
#51 | 2796173-field_layout-51.patch | 46 KB | tim.plunkett |
|
#51 | 2796173-field_layout-51-interdiff.txt | 10.52 KB | tim.plunkett |
#44 | 2796173-field_layout-44-interdiff.txt | 3.5 KB | tim.plunkett |
#44 | 2796173-field_layout-44.patch | 41.96 KB | tim.plunkett |
|
#42 | 2796173-field_layout-42.patch | 41.44 KB | tim.plunkett |
|
#42 | 2796173-field_layout-42-interdiff.txt | 11.13 KB | tim.plunkett |
#40 | 2796173-field_layout-40-do-not-test.patch | 41.05 KB | tim.plunkett |
|
#40 | 2796173-field_layout-40-combined.patch | 110.04 KB | tim.plunkett |
|
#40 | 2796173-field_layout-40-interdiff.txt | 7.8 KB | tim.plunkett |
#39 | 2796173-field_layout-39-interdiff.txt | 17.87 KB | tim.plunkett |
#39 | 2796173-field_layout-39-do-not-test.patch | 42.16 KB | tim.plunkett |
|
#39 | 2796173-field_layout-39-combined.patch | 111.15 KB | tim.plunkett |
|
#37 | interdiff-2796173.txt | 3.32 KB | tim.plunkett |
#37 | 2796173-field_layout-37-combined.patch | 110.06 KB | tim.plunkett |
|
#37 | 2796173-field_layout-37-do-not-test.patch | 39.73 KB | tim.plunkett |
|
#36 | 2796173-field_layout-36-do-not-test.patch | 39.05 KB | tim.plunkett |
|
#36 | 2796173-field_layout-36-combined.patch | 104.75 KB | tim.plunkett |
|
#36 | interdiff-2796173.txt | 1010 bytes | tim.plunkett |
#33 | 2796173-field_layout-33-do-not-test.patch | 39.05 KB | tim.plunkett |
|
#33 | 2796173-field_layout-33-combined.patch | 104.73 KB | tim.plunkett |
|
#30 | interdiff-2796173.txt | 11.79 KB | tim.plunkett |
#30 | 2796173-field_layout-30.patch | 43.04 KB | tim.plunkett |
|
#28 | interdiff-2796173.txt | 6.1 KB | tim.plunkett |
#28 | 2796173-field_layout-28.patch | 40.66 KB | tim.plunkett |
|
#27 | interdiff-2796173.txt | 9.37 KB | tim.plunkett |
#27 | 2796173-field_layout-27.patch | 37.84 KB | tim.plunkett |
|
#25 | 2796173-field_layout-25.patch | 33.21 KB | tim.plunkett |
|
#25 | interdiff-2796173.txt | 4.94 KB | tim.plunkett |
#22 | 2796173-field_layout-22.patch | 35.22 KB | tim.plunkett |
|
#22 | interdiff-2796173.txt | 4.43 KB | tim.plunkett |
#18 | interdiff-2796173.txt | 5.62 KB | tim.plunkett |
#18 | 2796173-field_layout-17.patch | 34.41 KB | tim.plunkett |
|
#15 | 2796173-field_layout-15.patch | 31.28 KB | tim.plunkett |
|
#15 | interdiff-2796173.txt | 7.64 KB | tim.plunkett |
#12 | interdiff-2796173.txt | 1.35 KB | tim.plunkett |
#12 | 2796173-field_layout-12.patch | 30.51 KB | tim.plunkett |
|
#9 | interdiff-2796173.txt | 16.86 KB | tim.plunkett |
#9 | 2796173-field_layout-9.patch | 30.29 KB | tim.plunkett |
|
#6 | interdiff-2796173.txt | 11.54 KB | tim.plunkett |
#6 | 2796173-field_layout-6.patch | 19.7 KB | tim.plunkett |
|
#5 | interdiff-2796173.txt | 18.73 KB | tim.plunkett |
#5 | 2796173-field_layout-5.patch | 17.79 KB | tim.plunkett |
|
#4 | interdiff-2796173.txt | 9.7 KB | tim.plunkett |
#4 | 2796173-field_layout-4.patch | 13.31 KB | tim.plunkett |
|
#2 | 2796173-field_layout-2.patch | 10.44 KB | tim.plunkett |
|
Comments
Comment #2
tim.plunkettThis does not let you switch between layouts, but it changes it from a single "Content" region to having "Top" and "Bottom".
More to come.
Comment #3
larowlanElegance in simplicity
Given there's so little code - does this need to be experimental? Does it even need to live outside field_ui?
If this was a service - could contrib extend it now to return new regions and layouts?
These could be constants?
Comment #4
tim.plunkett1) Honestly, it's much less code than I thought, but it's also not done. It really depends on the scoping of UX/workflow considerations, will bring it up with the UX team.
2) I think we'll leave it pretty close to this until #2296423: Implement layout plugin type in core, I'd rather only introduce one API. Another reason to make this experimental.
3) Could be, but this is just while it's experimental. Once it moves into Field UI, we can just add it directly where it belongs, no slicing or splicing needed.
Manage display should be working fine now. Going to start on Manage form display next, and also tests.
Comment #5
tim.plunkettWorks for forms now.
Comment #6
tim.plunkettOpened #2796877: Devise a mechanism for mapping old regions to new ones when moving between layouts and #2796885: field_ui.js hard-codes region names which are alterable for the @todos in there.
Either #2796581: Fields must store their region in entity displays or the larger #1786198: Make consistent regions in code for fields UI overview screens would make this a lot simpler.
Tests tomorrow.
Comment #7
swentel CreditAttribution: swentel at eps & kaas for MuseScore commentedYay!
My first reaction was 'Why so conservative'. Given that I just rewatched the Batman trilogy, this sounded very dramatic, but I'm a nicer guy than the joker :) I guess with #2296423: Implement layout plugin type in core being moved into core directly instead of a module, I was more or less assuming to move this functionality into core directly too. However, I understand that we're just in MVP phase, so it's better to play safe here. It's just a bit weird to see the same code (or at least same entry paths to alter) that happened in DS (don't know how panelizer/panels does).
Two things that stood out already in the patch:
This would mean that you can't disable Field UI anymore which is just a UI. You can disable field ui and still have your entity displays working too. DS works fine too without Field UI enabled. I think we need the same here too. I guess it's a step to far to create a field layout ui module for it.
Why loop over the components ? This looks weird. If we take the configuration from the layout, then we know which fields are going to be rendered. All other fields should automatically be hidden and not be rendered. this is what we do in DS This would mean the 'unset($build[$name])' can be removed too (I guess in the end #access FALSE would be better anyway instead of unsetting).
Another thought, but more of a semantic discussion. Field Layout sounds a bit poor. We're not really configuring layouts on fields, but entity displays, so entity_layout sounds better in my mind. But I don't want to fight to hard on this though. The reason is that I know that with the entity field api people we discussed whether to rename field ui to entity ui (I thought we had an issue already for that, but can't find it, guess we just wanted to ship D8 first heh)
Comment #8
tim.plunkettA lot of this code was inspired by DS, but I did take into account how Panels does things too, as I'm more used to that. The code should look familiar to both camps though.
As this is the fifth experimental module I've worked on (of the ten that exist), I'm getting pretty comfortable with the speed of iteration and ability to make changes during the experimental phase. Also, I've learned not to try to do too much in the first pass. This scope is not aggressive, but I think it is efficient.
1) Yes, this absolutely needs to change before commit.
2) I needed to loop over everything in order to handle real fields and extra fields. There may be a better way to do this, I need to play around with it once there is test coverage.
I picked "Field Layout" because I needed something to get started. I also remember the discussion about renaming Field UI to Entity UI (or Entity Field UI?), not 100% sure that "Entity Layout" is a better name, but I'm absolutely open to changing it.
Thanks for the review!
Comment #9
tim.plunkettI ended up adding a service like @larowlan suggested, while waiting for layout plugin to be integrated the list can now be swapped out.
Spent most of the day learning JavascriptTestBase. Turns out that if everything is broken, just add
$this->assertSession()->assertWaitOnAjaxRequest();
and it will all start working...Comment #10
swentel CreditAttribution: swentel at eps & kaas commentedHmmm, wasn't a huge fan of adding that service because of what you said in #4.2 which I agreed with initially, however you can swap it and then call the layouts if you have layout plugin module installed which would be your best bet, so yeah, I can live with it.
Or ... can me make an experimental core module dependent on the contrib layout plugin module ? :-) (/runs)
Comment #12
tim.plunkettIntegrating this with layout_plugin is the number 1 priority. But without something in core (like this) to use it, adding layout_plugin by itself probably won't happen. So hopefully this makes that happen sooner.
Fixing the hook_help. Not making a d.o handbook page until we settle on a name.
Copying this from the parent issue:
Comment #13
jibranI think other then module name it is pretty much ready. +1 for the service.
We should assert row weight before and after this.
Comment #14
tim.plunkettNeed to expand the issue summary, but I've filled in the remaining tasks.
Comment #15
tim.plunkettFixed a bug found while writing tests (yay!) and removed the dependency on field_ui.
Comment #16
swentel CreditAttribution: swentel at eps & kaas commentedIn DS we set the module weight to 1. I'm not 100% sure anymore why we did that, but I can at least think of one reason: extra fields might be added by other modules in the same hook, but could potentially run after this one. So either a module weight or a module_implements_alter is needed then. Shouldn't be part of this patch though, can be a follow up.
Comment #17
tim.plunkettWrote a hook_install, as well as a hook_entity_presave() so that newly added entity displays get default values.
Comment #18
tim.plunkettAttaching files is good.
Comment #20
dawehnerI'm wondering whether altering the form structure itself is the right thing to do. Doesn't that make it harder for other code to alter the form as well?
We are getting super oldschool now :)
Comment #21
tim.plunkett1) Yes, it does. Looking into how to better accomplish this.
2) Hah it does feel that way. But that's how recently committed traits have been documented, since they cannot use {@inheritdoc}.
Working on the test fail, \Drupal\config\Tests\ConfigImportAllTest is the worst.
Comment #22
tim.plunkettPosting a new patch with an attempt at hook_uninstall. Not sure if that fixes it, having issues running CIAT locally.
Also cleaned up some of the rendering code, and added assertions to prove it all works after you uninstall the Field UI.
Comment #23
swentel CreditAttribution: swentel at eps & kaas commentedBriefly talked with tim in IRC because I didn't understand the hook_install and entity_presave hooks very well. In short, the idea is that a layout is required, so on install, we need to configure the displays already. That's why there's a 'default' layout defined now, which can be replaced in future patches where say the node entity type then declares the node.tpl as its default layout for instance. The install and presave dance still feels a bit weird, but maybe we can come up with better ideas in the future, for now I'm fine with that just to get this in.
It's weird though that the uninstall hook is needed, I'd swear that CMI cleans up third party settings when uninstalling a module.
Comment #25
tim.plunkettAfter countless 15 minute local test runs of ConfigImportAllTest, I figured it out.
a) hook_uninstall was not needed.
b) The $entity->isNew() check was not sufficient. Something must have been messing with that elsewhere. Switching to check for the presence of our settings works.
c) The code to find all the fields was WAY overkill. Just stealing the approach from hook_install, works just as well.
Additionally, I realized that I was writing hook_install like it was hook_update_N. But the Entity API is allowed to be used in hook_install! So it becomes much simpler.
Comment #26
tim.plunkett@jibran clarified on IRC that he misunderstood what I was dragging. Since this patch only touches the custom tabledrag of Regions, and not of Weight, not adding test coverage for that.
Comment #27
tim.plunkettUpdated the test to use entity_type where possible.
Also added coverage for entity form displays.
Comment #28
tim.plunkettFinished the tests.
Comment #30
tim.plunkettRemoved the pointless 1col_stacked layout.
Renamed 2col to twocol, since Html::cleanCssIdentifier() strips the leading number.
Allowed layouts to provide a library, and added a basic CSS file for twocol.
Wrapped the regions in a layout template.
Comment #32
yoroy CreditAttribution: yoroy at Roy Scholten commentedWe reviewed current status during today's UX meeting: https://youtu.be/vRNPFs9fISk?t=37m16s (with screenshare demo)
- This is exciting!
- Current patch uses existing user interface paradigms, which results in a sub-par ux but don't let that hold back an initial commit
- The scope seems clear enough: should we open a separate issue for exploring UI ideas for this?
- Go Tim!
Comment #33
tim.plunkettThis is now hard blocked by #2796581: Fields must store their region in entity displays.
It incorporates the patch from that issue, which is why it's so big.
@yoroy I think webchick is working on a separate UI issue, it will be linked from here. Glad to hear excitement!
Comment #36
tim.plunkettThis needed the same fix as #2796581: Fields must store their region in entity displays.
Comment #37
tim.plunkett#2796581: Fields must store their region in entity displays is now RTBC.
This fixes field_layout.install, and addresses @dawehner's #20.1 by using #group for forms.
Comment #39
tim.plunkettReroll and fixed bugs.
Comment #40
tim.plunkettMoved to a single template file instead of nesting region templates in a layout. This was requested by themers, is more similar to Display Suite, and is what Layout Plugin would also expect.
Comment #42
tim.plunkettBlocker was committed!
Moving the code more towards layout_plugin-esque code.
Comment #44
tim.plunkettUnfortunately, by switching from nested #theme_wrappers to a dedicated single #theme, we're now hitting #953034: [meta] Themes improperly check renderable arrays when determining visibility.
Adding a workaround for now.
570f95a Restore proper 'default' handling
365c466 Restore markup to default template.
60c5a51 Add workaround for #953034
Comment #45
tim.plunkettComment #51
tim.plunkettFixed the bugs, and allow templates to be placed arbitrarily
Comment #52
tim.plunkettNeeded to change getLayout() to remove the default value handling, there are other cases where we need to be able to check if something has a layout or not.
I think this is very close. Code reviews would be very welcome!
Comment #53
dawehnerJust some quick feedback while not falling asleep.
Conceptually this is an
array_walk()
not a map. A map stores values.I'm wondering whether we should move this method via
hook_module_implements_alter()
to the end.I'm wondering whether one or multiple library support is what one actually needs. I guess it just makes sense to have a single one?
It would be nice to document why we are doing this here. Why are base fields treated different
Should we support no provider?
It is interesting that we support theme functions even ... maybe we don't want to do so in the first place
Is the plan to move this method and use the categorize plugin manager, wants the layouts are pluggable.
At least this test doesn't have to run in the JS environment or does it?
Comment #54
tim.plunkett1) Very true :)
2) @swentel had the same thought in #16 but asked that it be a follow-up.
3) I also considered giving it support for multiple, but you could just write one library with multiple dependencies. Also this is how layout_plugin does it.
4) Non-configurable fields should not be moved, as they are usually handled by something like preprocess (see template_preprocess_node moving everything around for submitted by).
5) Once they become plugins, they will automatically be assigned a 'provider' by the plugin system, so I'm hardcoding it to mimic that.
6) Well we just do
$definition['template'] = strtr($definition['theme'], '_', '-');
if you provide 'theme' but no 'template', so I'm not sure it matters.7) Yes it is. Adding an @todo.
8) It doesn't need it, but should I make a separate BTB just for this method?
Comment #55
dawehnerWe have a bit of a problem already because javascript tests aren't executed in parallel at the moment, so it slows down test results more than normally.
Comment #57
tim.plunkettarray_walk fails because it takes the array by reference.
I realize array_map is slightly less semantically correct, but the docs do say
and that's precisely what I'm doing. I don't think the fact that we don't care about the return value makes its usage any less valid.
Comment #58
dawehnerInteresting. Oh PHP
Comment #59
tim.plunkettDries used the name "Field Layout" in his keynote, and nothing came of the other suggestions in the parent issue.
Comment #60
samuel.mortensonWhen manually testing this, switching layouts the first time (to "Two column") threw the notice
Undefined index: type in Drupal\field_ui\Form\EntityDisplayFormBase->determineComponentAction() (line 664 of core/modules/field_ui/src/Form/EntityDisplayFormBase.php)
. Every subsequent change did not throw Notices.I manually went through the entire flow of using a Two Column layout and re-positioning fields, and that seems to work as expected. Nice job!
Looking at the code:
One alternative to doing alters would be to attach a custom View Builder using hook_entity_type_alter(), not sure if that's a fit or we need that complexity, but it's what Panelizer ended up doing in its Drupal 8 port. Panelizer has to deal with Entities being "Panelized" or not, and Field layout is implicitly enabled for all Entities, so this may not be relevant here.
Panelizer accomplishes this by implementing hook_form_entity_view_display_edit_form_alter() and attaching a new submit handler which makes sure the third party settings are saved appropriately. Again, just pointing this out in case it's easier to go an alternative route.
Comment #61
tim.plunkett1) Done. That was copied from DS.
2) Done.
3) I copied the CSS from D8 Panels (it uses flexbox). I can go back to the D7 float/width approach.
4) That's just Field UI being Field UI. It's nothing we're doing here.
5+6) These suggestions are opposites :)
5 is saying "instead of altering, swap out the entity handler"
6 is saying "instead of swapping out the entity handler, use an alter"
I'm quite happy with how the class/form swaps are working in this case. And I think swapping out EVERY entity view builder is a mistake, since only one module can do that...
Comment #62
samuel.mortensonYeah, notes 5 and 6 were just me reading through the code and thinking "I wonder why Panelizer does the opposite?". If you prefer the methods you used I'm good with that.
We can keep flexbox and add max widths and margins, I can take a stab at the CSS required for that. I assume that day one people are going to start turning this on for Forms and will expect multiple columns to work regardless of the admin theme.
Comment #63
tim.plunkettThe notice is actually another bug from #2796581: Fields must store their region in entity displays, opened a new issue for it: #2811467: Add backwards compatibility layer for entity display default config that lacks 'type'
Comment #64
samuel.mortensonThis change adds a 10px margin between the left and right column, and prevents columns from being wider than 50% (which would prevent flexbox from displaying two columns).
Comment #65
tedbowThis is going to be great! Just a couple of comments
This function seems like it would be better in a service. Just easier to find and it is pretty complex.
This is being called for every entity save. Seems like it would be more efficient(though less elegant) to implement 2 hook_ENTITY_TYPE_presave's
Comment #66
tim.plunkettThanks @samuel.mortenson!
#65
1) It is complex, but exposing it as a service would be wrong. We don't want others to consume this, it's not an API.
2) I did clean this logic up, thanks. Feels good to stop cheating with get()
3) Good point!
Comment #67
dawehnerIn that case, just create a class and call it. We are doing this too less, because we are scared by OOP still.
Comment #68
tim.plunkettFair enough. Converted, wrote unit test, refactored.
Now with the exception of _field_layout_entity_display_presave(), all of field_layout.module is hook implementations
Comment #69
dawehnerNice!
Comment #70
tim.plunkettJust fixing two coding standards nits that were bugging me.
Comment #71
tim.plunkettStarted in on the conversion for #2296423: Implement layout plugin type in core, and found a couple changes that can be made now to minimize future changes.
Comment #72
tim.plunkettOkay the conversion to layout_plugin was actually very easy and not as big as I'd thought.
After discussion with @tedbow and @xjm, changing tack.
Now that we know this code will work for field_layout, we'll try to get layout_plugin in on its own first. Any changes to that code will be reflected here to ensure it continues working.
I will also run Panels and DS tests locally with this code to ensure it continues to work.
Comment #74
tim.plunkettComment #75
dsnopekNow that this is using layout_plugin, you need to store layout configuration here, otherwise layouts that have it will explode!
Comment #76
tim.plunkett#2796581: Fields must store their region in entity displays was reverted, so all of this is postponed.
Comment #77
tim.plunkettComment #78
tim.plunkettPosting a combined patch of this, #2796581: Fields must store their region in entity displays, and #2296423: Implement layout plugin type in core.
Interdiff only includes changes made to field_layout code, not the updates from the other issue, so it's not a true interdiff.
Will mark postponed again after the tests pass (or don't?!)
Comment #79
tim.plunkettGreat
Comment #80
tim.plunkettComment #81
tim.plunkettPosted an updated patch. No explicit changes to field_layout, just updating based on changes in the blocking issues.
Comment #83
tim.plunkettWorking on the fails in a test-only issue.
Comment #84
tim.plunkettOne blocker down, just layout_plugin left. The last fail was in layout_plugin.
Comment #85
tim.plunkettOkay just waiting on #2296423: Implement layout plugin type in core
Comment #86
tedbowA few nits
I think this should be "Per view mode" not "Per-view mode". I guess it could be Per view-mode"
Why have "content" here?
Field info file says "Field API to add fields to entities like nodes and users."
Could be:
"The Field Layout module allows you to arrange fields into regions on forms and displays of entities such as nodes and users."
"that has a layout"
Missing @param comments
Missing comma at end of line
Missing comma at end of line
Unused import statements
Comment #87
tim.plunkettThanks for the review!
1) It's "per view mode" elsewhere in core, going with that.
2) +1
3) Done
1) Fixed
2+3) Hah, those were copy/paste from HEAD :) fixed
Unnumbered) Fixed
Comment #88
dawehnerI'm wondering whether we could get rid of having a FieldLayoutBuilder for forms and one for views
What about using
$form_state->getValue(, $stored_layout ? ... )
Maybe we could explain why
I really like this test
Comment #89
tim.plunkett#88.1, do you mean separate classes for form and view?
Comment #90
tacituseu CreditAttribution: tacituseu commented1. Since layout ID is expected to be unique and Panels already uses 'onecol' and 'twocol', it would cause conflict/unexpected behaviour after Panels gets installed, maybe prefix them somehow.
Should modules be responsible for assuring their layout IDs are unique ?
2. After uninstalling module providing layout used on view I get this when viewing a node:
So another question is: should module providing layouts be responsible for cleaning up 'field_layout' on affected bundles ?
Comment #91
tim.plunkett1) Yes, every module is responsible for namespacing their plugin IDs, neither Panels nor this experimental module do that yet.
2) I'll have to look at that, the dependency system is supposed to prevent that from happening.
Comment #92
tim.plunkettStill postponed, but updating for recent changes to the layout API.
This should address #90.2
Comment #93
tim.plunkettWe're now unblocked! Layout API is now in core.
Comment #94
jibranOther then the module name I have unable to find anything objectionable in the patch.
Comment #95
xjmComment #96
xjm@tim.plunkett mentioned that @amateescu's feedback and review could be valuable here. We already have review from a (former) Field maintainer, so this might be optional (thence leaving RTBC), but also tagging.
Marking for relevant committer reviews as well.
Comment #97
xjmFeature request really, but definitely a major one.
Comment #98
xjmThanks @jibran for the screenshots. That is great.
Comment #99
swentel CreditAttribution: swentel at eps & kaas commentedIIRC, we have a hook_form_BASE_FORM_ID_alter (so we can do form_node_form_alter). Wondering whether we could introduce someting alike for contentEntityForm (or maybe that even exists already).
Should not hold up commit, but can maybe be some sort of optimisation ?
Comment #100
tim.plunkettOne line down from that is
It's the first condition in order to filter out that as soon as possible.
There is no mechanism for finding and altering all content entity forms.
Base form IDs for entity forms usually contain the entity type ID (like node, user) but can also be completely arbitrary in custom code.
Comment #101
jibranI thought about the name. Here are the few names, some are already suggested by @tim.plunkett earlier in the issue:
Given all that, I think field layout is the best fit here.
Comment #102
aspilicious CreditAttribution: aspilicious commentedQuick First Time review on my phone, are contextual links working in the frontend? I had to add some lines in each template to make it work (for ds)
Comment #103
tim.plunkettThey work the same as they do before. So node works, but user doesn't (because it doesn't print title_prefix/title_suffix).
This has no effect on any of that, because the layout represents the {{ content }} part of the entity template.
Comment #104
phenaproximaGave this a read-through. I found nitpicks, but nothing that should block commit, IMHO.
Not sure this should be committed as-is :)
Nit: Should the call to getInstanceFromDefinition() perhaps be on its own line for readability? Not sure how that would jibe with the coding standards, though...
&$form should be type hinted.
<3
Not a big deal, but could there be an explanation of what it means for a region to be the "default" one?
Should probably say "layout plugin ID", for clarity.
Forgive my ignorance, but what is the difference between this method and setLayout()? The presence of $layout_settings means this method is capable setting more than just the layout ID. Under what circumstances would I call this method versus setLayout()?
So setLayout() accepts a LayoutInterface, and this method returns a LayoutInterface, which implies that they are inverses of each other. Yet they are named differently (one uses the word "plugin", the other does not). Why the inconsistency?
I wish this were a little more descriptive. What is the purpose of the shared code it's providing?
I'm not a huge fan of the fact that the trait is implicitly trusting the developer to inject the layout plugin manager, but I can live with it.
Parameters are not documented, and $form can be type hinted.
Never seen this before. What is it?
Missing descriptions.
Not a problem, really, but why is this public and static?
Comment #105
tim.plunkett1) Fixed
2) Discussed this with @dawehner before, IMO makes more sense to leave it this way.
3) form.api.php defines this as
function hook_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
, so not adding a typehint.5) This is actually just making a protected method into public one, the docs are copied from \Drupal\Core\Entity\EntityDisplayBase::getDefaultRegion()
6) Fixed
7)
setLayout() is a shortcut method:
setLayoutId() is used when you don't have a whole plugin object. For example,
8) Leftover from many many rounds of refactoring. Fixed.
9) It's a trait. EntityViewDisplay and EntityFormDisplay both have shared code via a base class, EntityDisplayBase. Due to how PHP inheritance works, we have to rely on a trait here. This code will eventually be moved into the base class.
10) The protected property is defined on the trait, it's safe enough.
11) These are standard callbacks. The docs standard says
That's why it starts with "Ajax callback / Submit handler" instead of the usual format.
12) They're config dependencies specified by a plugin. If you grep or google for it, you'd see it is documented on \Drupal\Core\Config\Entity\ConfigDependencyManager.
13) It's a test method, the @param is generally enough.
14) All PHPUnit assert methods are public and static.
NR for the method name change.
Comment #106
phenaproximaLooks good to me. Back to RTBC with you!
Comment #107
xjmI plan on doing a more thorough review of this soon if possible, but I definitely think the issue needs to wait for product manager signoff. Meanwhile, NR for points 1, 5, and 6 above.
Thanks everyone!
Comment #108
xjmOh, one more thing. Was the point about plugin namespacing for this module from #91 point 1 addressed? That's also part of "try to get the name right" and one of the things we struggle most with.
Comment #109
xjmCan we also add a screenshot of the UI? This would probably help product manager review.
Comment #110
swentel CreditAttribution: swentel at eps & kaas commented@xjm
Well, tim answered in #100 - I was more thinking out loud here. Since Tim has more knowledge of the form system, I'm fine with this :)
Comment #111
tim.plunkett#88
1) @dawehner and I discussed this on IRC, the shared logic of FieldLayoutBuilder outweighs the benefits of two classes IMO.
2) Changed
3) Expanded the comment
4) Thanks!
Included are some screenshots. First what's in HEAD, the selector, then with a 2 column layout with no settings, then with a test layout with settings.
Comment #112
xjmAdding one of the UI screenshots to the summary.
Comment #113
xjmA couple annotated screenshots to show how it works.
Comment #114
xjmComment #115
amateescu CreditAttribution: amateescu for Pfizer, Inc. commentedThe patch looks really awesome to me! I have to say that I did not have time to do a very thorough review, but this is an experimental module after all so I think it's ok to go with what we have :)
I just found two things and probably the first point needs to addressed before commit:
I assume there's some code in the overridden entity view|form display classes that adds something to the config entity, should we also have an uninstall hook to remove that information?
This variable name (and comment) is a bit confusing, since we usually refer to "configurable fields" as the ones that use dedicated tables to store their data (i.e. the ones that are handled by FieldConfig and FieldStorageConfig).
So my initial reaction to this code was: "wait, why are we removing base fields?" :)
Comment #116
tim.plunkett1) Exactly, the overridden entity class has this:
Because the storage mechanism is third party settings, this is cleaned up automatically on uninstall by \Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval()
We have coverage of this from the beloved \Drupal\config\Tests\ConfigImportAllTest
2) Upon rereading that code, I agree it is confusing.
Hopefully renaming the variable and moving the comment to where the decision is made will improve clarity.
Comment #117
phenaproximaNice change. Much clearer. Go back to the RTBC whence thou came!
Comment #119
tim.plunkettRandom fail in Drupal\system\Tests\Update\SiteBrandingConvertedIntoBlockUpdateTest. Requeued, and re-RTBC'd
Comment #120
tim.plunkettComment #125
xjmThanks @amateescu and @tim.plunkett.
Some parts of #107 and #108 are still not addressed as far as I can see.
Comment #126
xjmI guess mostly just #108 and whether or not DS is good to go. Edit: I assume it is if @swentel is okay with it, so really just #108.
Comment #127
tim.plunkettWhen I went to address #108 I realized how many places had 'onecol' hardcoded. All of them were just to work around the layout not being set in time for init(), so I shared the logic from preSave() to cover it.
Then I prefixed the plugin IDs.
Also changed the arbitrary test IDs to be different from the real ones to be less confusing.
Comment #128
tedbowThis look good to me. #108 seems to be addressed and #88 which it references.
Comment #130
tim.plunkettTestbot fluke
Comment #132
tim.plunkettComment #134
tim.plunkettThis is still really RTBC, but the php 5.5 bots are nonfunctional. Queued it with 5.6 to prove it passed.
Leaving a NR to stop the false fails.
Comment #135
samuel.mortensonIs there a testbot issue for the PHP 5.5 test failures? I'm getting the same test results in #2815221: Add ability to use Quick Edit to the latest_revision route.
Comment #136
tim.plunkettHoping testbot is better now?
Comment #137
tim.plunkettComment #138
webchickWe discussed this with @Bojhan, @yoroy, @Dries, and @Gábor today on the Ideas Queue review meeting, and were infinitely ecstatic about this patch going in. :) It fits a great need, and the implementation works as advertised.
Our recommendations were:
1) Adding more layouts to choose from (this is handled by #2840832: Add layout templates based on Panels layouts to core, and has become a "must-have" in the meta issue). This should go in *after* this patch, however, since it could be prone to bikeshedding. :P And unlike this patch, it can go in any time before RC.
2) Following up from #1, improving the selection of layouts so people have some idea (ideally a visual representation) of what "3 column stacked" vs. "2 column blocks" means.
Removing the product manager review tag. SHIP IT! :D
Comment #139
xjmTo clarify, once the experimental module is added as alpha, it can have feature additions during any phase of the release cycle (including RC and patch releases), until it reaches beta stability.
Comment #140
tim.plunkett#2660124: Dynamically build layout icons based on well formed config is intended to address #138.2, and is listed on and linked from the plan issue
Comment #141
xjmI scanned the patch, not including the test coverage. A few questions and observations. I tried to clarify whether they're in scope or not.
UX gate followup: I think this label could probably be improved. Also, non-blocking nit: I believe this should be "Per-view-mode field layout settings".
UX gate followup: Review/improve the module description.
Docs gate followup: This handbook page needs to not be totally empty. ;)
Non-blocking nit: "one-column layout", "two-column layout".
Followup: If we don't get a frontend framework review before commit, then it should probably be a followup on the roadmap specifically for this module, the same way we have one for the overall theme implementation for the layout API itself.
Non-blocking nit: What kind of shared code?
So these traits are pretty massive, especially the form one. What's the reason that they are traits? I guess so that the forms can extend both them and the base class? What's the case for reusing the traits though? (The answer to this might be in the patch somewhere; just struck me as a little unexpected.)
Non-blocking: I had a kneejerk that these should be
{@inheritdoc}
but of course it's a trait so it's not inheriting anything. D'we have any examples of this documentation pattern elsewhere?Non-blocking nit: Per https://www.drupal.org/node/1354#order @todo should come after @see.
I confirmed that this @todo is already a "Should" on the overall Layout API roadmap.
So do we expect a followup to remove it once Drupal requires PHP 7? This might be an academic question, but D9 should have PHP 7 as a minimum requirement.
Non-blocking nit: I am pretty sure
{@inerhitdoc}
can't be used with anything else still. There's some issue about this somewhere.Non-blocking nit: "Constructs a blah" is normal I think, redundant and silly as that is.
Non-actionable observation: Neat.
Non-blocking pontification: Should these be constants? Are they elsewhere in the Entity API? Come to think of it, are display contexts extensible?
All the stupid non-blocking docs stuff could be a "Should" have followup like "Clean up dumb docs nitpicks in blah," or incorporated in the patch, as per your preference.
Comment #142
xjmMeant to do this. This is still ready for framework manager review, though.
Comment #143
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI've started reviewing this and will post more comments today. Just a quick question in the meantime...
Is there an already existing issue to discuss how these get organized? I can imagine various other modules and/or themes implementing layouts labeled "Two column" and in the same category. In which case, the site builder would see multiple options with the same labels, which is confusing. I don't know that #2660124: Dynamically build layout icons based on well formed config would resolve this either, since the icon previews might look very similar or identical as well.
The reason I bring this up during the "framework manager review" is that if these were being added to a non-experimental module, then we could reasonably tell contrib that it's their responsibility to not use the same labels as what's used in a core module. But I don't know if it's as reasonable for contrib authors to monitor experimental modules for that kind of clashing.
Related, why should "Field layout" be the module that owns the "Two column" label name? Seems like it should be possible for me to not enable field layouts, but enable some other kind of layout module (e.g., per-node layouts, or something else in the Panels ecosystem), and still have some layout on the site that's labeled "Two column". But if I do, and then enable field_layout.module, now I have two layouts with that label. Which makes me think that maybe these layouts belong in some other place than field_layout module, so that they can be available to the system even when field_layout module is not enabled? #2840832: Add layout templates based on Panels layouts to core says "Decide where they should live, currently in layout_discovery". Should we therefore move these to
layout_discovery
too?Comment #144
tim.plunkettWorking on @xjm's feedback.
#143, I would fully expect field_layout to ship with zero layouts once stable.
This was discussed during the product manager review, and we chose not to block this issue on adding more layouts and in the correct location.
Another essential issue for this is #2822758: Introduce a way to distinguish between different types of layouts
Comment #145
xjmI think this probably needs a CR (both for the addition of the feature, and for how contrib/custom code might implement or extend it)?
Comment #146
effulgentsia CreditAttribution: effulgentsia at Acquia commentedIs there an issue comment about this explaining why? I understand not blocking on more layouts. But why not get the layouts proposed in this patch into the correct location? Moving them later probably also means renaming them from
field_layout_*
to something else, which means either writing an upgrade path or allowing sites that have this module enabled to break. I propose renaming them tolayout_*
(is core allowed to claim the "layout" namespace like that or would that break something already existing in contrib?) and moving them tolayout_discovery
. I could potentially see us wanting to move them fromlayout_discovery
to eithersystem
,/core
, or somewhere else, but so long as they're namespaced tolayout_*
, they should be movable without a rename or upgrade path, right?Comment #147
effulgentsia CreditAttribution: effulgentsia at Acquia commentedWhen I uninstall the module, even though the UI tells me that the various view and form display configs will be updated such that all the data from this module will be lost, that does not actually happen. Those configs continue to have references to the uninstalled module. I suspect that would potentially break any place that checks schema, such as config translation, except I can't test that due to #2546212: Entity view/form mode formatter/widget settings have no translation UI. However, if I export the site config, then make some change to one of the displays on the site, then do an import and sync, I get errors like:
Configuration core.entity_view_display.node.article.teaser depends on the Field Layout module that will not be installed after import.
Comment #148
tim.plunkett#146 It's my understanding that when adding this module, the changes should be limited to this module. Also, it's experimental, so I have no thought toward upgrade path if we rename whatever.
#147 This means that there is a generic core bug in the handling of third party settings, which I am surprised by.
\Drupal\Core\Config\Entity\ConfigEntityBase::onDependencyRemoval() is supposed to handle this.
Comment #149
effulgentsia CreditAttribution: effulgentsia at Acquia commentedNot a blocker for committing this as an experimental module, but the implementation of this function seems very different for 'form' and 'view'. Given that, why make it a single function? Maybe better to have
buildForm()
andbuildView()
(orentityFormAlter()
/entityViewAlter()
)?Comment #150
tim.plunkett#141
1 Fixed
4 Fixed
6/7 Improved docs
8 See FormStateValuesTrait and RevisionLogEntityTrait for examples. Only appropriate for traits.
10 Fixed
11 Fixed
13 Removed due to #149
#147 This is covered by \Drupal\system\Tests\Module\InstallUninstallTest and \Drupal\config\Tests\ConfigImportAllTest.
#149 Addressed. This was brought up numerous times and considered okay to have, but I'm tired of defending it.
Comment #151
xjmI added bullets for #141 1, 2, 3, and 5 to the summary of #2795833: [plan] Add layouts to entity displays (both form and view). The first part of 10 isn't actually addressed but I assume the answer is "Yes, but it's not worth filing an issue now for something so far off" so I added that as a bullet under "Could".
I think #147 and the upgrade path question probably merit further discussion.
Normally we do not require upgrade paths for alpha modules (and beta is best-effort) but we might choose to given that we also agreed to do so for #2833976: How Panels, Display suite, Layout Plugin, Layout Discovery and core get to a stable layout plugin in core because of the impact on contrib. We can discuss that in a followup and I don't think it needs to block commit for the prototype.
However, #147 could suggest a data integrity problem, which is one of the things that can block the initial commit of an experimental module, so I pinged @alexpott for feedback on it. Leaving at NR for that.
Comment #152
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI don't think that's necessarily true. If the development of this module surfaces an architectural requirement, such as the existence of a layout that needs to be available even when this module is not enabled, then I think it's fine for this patch to include that layout in the location that makes most sense. Or, a child issue could be made for adding just that layout.
The problem is that I think a rename is quite likely for the reasons in #143. And not providing an upgrade path could put the site in a pretty bad state. For example, if the site has a global custom block on all admin pages (either in the admin theme, or the site chooses to use the default theme for the admin pages), then as soon as you deploy code with a renamed layout, then after a cache clear, visiting any admin page on your site results in a fatal "The website encountered an unexpected error. Please try again later." error. Which means you can't even uninstall the module via the UI anymore, so even following the instructions in https://www.drupal.org/core/experimental#alpha to uninstall and reinstall is difficult for users without Drush.
What if we just rename them in this patch from
field_layout_*
tolayout_*
in order to not create avoidable future upgrade path work for ourselves? We can leave them in the field_layout module for now, and maybe add @todo comments tofield_layout.layouts.yml
explaining why they're not namespaced to the module and a link to the issue for discussing where to move them to. Or do we foresee problems with existing contrib modules if we start using thelayout_*
namespace for layouts?Comment #153
tim.plunkettI don't think we have that need yet.
The product managers and UX team agreed to not block this on the "add more layouts" issue.
If you as a framework manager want to hard-block this issue on that, just do that. But I don't think we should be adding layouts to layout_discovery or purposefully misnamespacing them in this issue.
Comment #154
tim.plunkettI finally reproduced #147.
On uninstall, config deletion takes place before the module's hooks stop firing.
field_layout_entity_type_alter()
is still in effect, which means ourFieldLayoutEntityDisplayTrait::preSave()
continues to fire, which ensures it has a layout.Wrote a test for this, and a hacky fix.
A major problem with this fix: there's no way I can see to clean up that state value after we don't need it anymore. This would cause issues on reinstall.
Comment #156
tim.plunkettIdeally this wouldn't take affect during uninstall, but I'm guessing modules rely on such alters still running. And no idea of how to opt out of it (especially since it's cached!)
Comment #157
tim.plunkettOkay came up with a more tenable fix.
field_layout_install() no longer depends on preSave() to magically do what we need, it's now handled explicitly.
preSave() can now be much simpler, and we'll let init() handle the rest of the cases.
Also just renamed according to #152, I think that's a reasonable compromise.
Everything since the last time it was RTBC has been addressed.
Comment #159
tim.plunkettWhew, not a real failure, just missed a spot
Comment #160
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'm liking #159 a lot! Thank you. Uninstallation is much nicer now.
But one problem I'm still seeing is that when you install Standard, then install Field Layout, then uninstall Field Layout, then the
node.article.default
entity view display gets itscomment
component moved to hidden. If you then go to/admin/structure/types/manage/article/display
to remake comments visible, then after another install/uninstall of Field Layout, it gets again reset to hidden.I suspect the problem is somewhere within the
ConfigManager::getConfigEntitiesToChangeOnDependencyRemoval()
pipeline, and not within this patch's code. It's just that we have no other place in core that adds third party settings to EVD entities, and this patch is possibly surfacing a general bug with that. However, even if that's true, https://www.drupal.org/core/experimental#requirements lists "No known security or data integrity issues." as a requirement, so setting to NW for that, even if the only fault of this patch is that it's surfacing a data integrity issue in a hitherto unexercised API. Sorry.Comment #161
effulgentsia CreditAttribution: effulgentsia at Acquia commentedYay, thank you for these names :) While #160 is being sorted out, I'd love +1s here from maintainers of 8.x versions of any of the Panels/DS/Layout and related modules. I want to make sure the "layout" namespace for layout IDs is safe for core to claim. IMO, it's correct for core to claim it, because core is where
Drupal\Core\Layout
is, but if we have existing contrib that would conflict with that, I'd like to make sure we know about it.Comment #162
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'll discuss with the other committers, but possibly the only hard-blocker for this issue might be to verify that it's a generic bug with uninstallation of a module that uses third_party settings, write a test that demonstrates that generic bug, and file a Critical issue with a patch with that failing test. Then potentially, if the release managers agree, fixing that Critical might not need to block this module from being committed.
Comment #163
tim.plunkett#161, pinged other people, but there is no Layout module right now (nor plans for one), so this namespace is ours for the taking.
#160/162, the culprit is \Drupal\Core\Entity\EntityDisplayBase::onDependencyRemoval(). Since the comment display is tied to the output of
core.entity_view_display.comment.comment.default
, it's incorrectly determining that comment is being removed (which isn't true). Looking into how bad the logic bug is, but at least this is isolated within the entity display system.Comment #164
tim.plunkettOpened #2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation. Seems at this time to be isolated to comment fields, specifically when using CommentDefaultFormatter.
Comment #165
japerryRe: layout namespace: from the panels side I don't have any issue with this. I don't think panelizer is affected either.
Its possible some sites may have used the namespace to create layout features? Not sure we want to really accommodate for that edge case though.
Comment #166
tim.plunkettTurns out that other issue is a pre-existing bug, and has been marked critical.
Hopefully!
Comment #167
xjm#2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation does still need to be a blocker, because this module will expose it more. But fortunately it is RTBC so hopefully it will be in soon!
Comment #168
xjmYeah, there's always the risk of this when we add new code, until the day when core properly namespaces everything. But this is an acceptable risk; the best practice for custom code is also to namespace its stuff. We do need the CR though so that developers are informed (for this and everything).
Thanks @japerry!
Comment #169
xjm#2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation is committed to 8.3.x now, so this is no longer blocked on that.
Comment #170
xjmDoes #2843074: Stale dependencies passed to onDependencyRemoval() result in data loss on uninstallation address #147 as well as #160, or do we still need the "hacky fix" that was mentioned previously?
Comment #171
tim.plunkett#157 removed the hacky fix, and is even better to what we had before that, so the latest patch is still what we want.
Comment #172
xjmThanks @tim.plunkett.
I tested manually and confirmed that the full config export is restored to its original state on uninstallation, reinstallation, and a second uninstallation.
However, I'm now encountering the following behavior:
/admin/structure/types/manage/article/display
, expand "Layout settings", and select the "Two column" layout. Save./admin/structure/types/manage/article/display/teaser
. Select "Two column" and Save./admin/structure/types/manage/article/display
and try to enable the two-column layout again. The form is again not updated.Comment #173
xjmWhoa. And now when I uninstalled
field_layout
, all the fields except the comment field disappeared from my teaser display. Something is very wrong.Comment #174
xjmI noticed #2843707: Default config for entity displays does not match what is saved in the process of trying to debug this. Not sure if it's related or just an incidental and harmless config mismatch.
Comment #175
xjmAh, I've figured out what is happening in #173. When I switch the teaser layout to the two-column one, the region is updated to
left
instead ofcontent
. But when I uninstall field_layout, it does not repair the teaser to switch it back to thecontent
region. So the display edit form no longer has a definition for the layout provided, and it appears there are no fields to configure since they are assigned to a region that does not exist.The fields are still actually displayed on the teaser because they are falling back to the default region, but it's impossible to configure them anymore on the display. That's probably commit-blocking since the module does not uninstall completely and leaves the display in a broken state for the site builder.
Comment #176
xjmAlso, if you reinstall
field_layout
after #173, the content actually disappears from the display when it is viewed as well, which would be a critical issue for sure. To get your fields back at that point, you have to switch it back to the layout that was being used before the previous uninstallation (which is no longer recorded anywhere).I think I also figured out what was happening in the first half of #172. There is some AJAX behavior that happens when you change the layout setting, with a throbber. It does not appear to change the visible form. If you hit the "Save" button before whatever the throbber is doing is complete, your change to the layout is not saved. That seems like a major usability bug, not necessarily blocking for the prototype/initial commit, but maybe blocking for it to be stable at least.
Comment #177
xjmWhen we fix #173, we should also test the fix when the layout being removed is provided by a different module than
field_layout
. The regions in each entity display depend on the layout that is used with those regions. Layouts might be added and removed from many different places, and we need to figure out what the expected dependency management is when those layouts go away.Comment #178
xjmComment #179
xjmI added #172 / second half of #176 to the followup roadmap.
Comment #180
xjmReviewing this against https://www.drupal.org/core/experimental#requirements.
and
Test coverage is excellent.
Still needs the CR. The rest is covered on #2795833: [plan] Add layouts to entity displays (both form and view).
The UI is being added to our existing minimally validated field UI. :P The product/usability team signed off on it being sufficient for this experimental module.
This is what @effulgentsia was getting at in #152. The module provides third-party settings that simply map to the layout ID and plugin. It also provides two default layouts, named beginning with "layout", which an
@todo
to move them. I'm a little unsure about the mismatch between the layout machine names and the module name -- will that have any impact on owner/provider/dependency calculations? But it sounds like they're not going to be there very long in either case.I think the module name is fine.
Covered already above, barring any additional ones in @effulgentsia's further review or the fix for #175.
I've added a couple items for usability and accessibility testing. For what it's worth, I find the collapsed layout fieldset at the bottom of the form difficult to discover, especially since it's buried after the WTFy "Custom display settings" fieldset on the default display. So I added that to the "Should have" followups as well.
#2795833: [plan] Add layouts to entity displays (both form and view) is in good shape. There are a few items in it that need to be sorted/clarified; see #2795833-25: [plan] Add layouts to entity displays (both form and view).
So, for release management signoff, the outstanding things I think we need to address are:
I added these to the summary as remaining tasks. Thanks!
Comment #181
tim.plunkett#180.1
#173 uncovers two bugs!
Once field_layout is stable, it's my intention to merge it directly into EntityDisplayBase and Field UI, and uninstall wouldn't be possible. If you wanted to stop using a layout, you'd just switch back to the one-column.
So that's exactly what field_layout_uninstall() needs to do!
But that still doesn't fix the regions for each field. Which confused me, since I know we have code to do that, AND coverage for it.
The problem is that I put that code in the submitForm(), making it UI only.
Updating the kernel test first makes everything fail, and then moving the logic directly into the API fixes it.
#180.2 This is fine. It's convention only, no code cares about this. It's why each plugin has a "provider" tracked for it.
#180.3 @todo for tomorrow
#180.4 @todo for tomorrow
#180.5 TBD
Comment #183
tim.plunkettBad interdiff on my part, this was in the last patch!
Comment #184
xjmCoolness, glad those bugs were straightforward to fix.
I can see how that would make sense, but there would still potentially be the need for a smooth upgrade path/process for that transition. It's similar to the questions we had to address for the Layout plugin itself; in fact it's also related, since the dependency for entity displays was one of the compelling reasons for the layout plugin API to be in
core/lib
. For this module the question is much simpler smaller in scope: we already plan to move the layouts to the parent module, and almost everything else is this patch is internal API. We should add a point for moving the code to the end of the field layout roadmap. For now I just added a note to the summary here.Comment #185
tim.plunkett#2795833-32: [plan] Add layouts to entity displays (both form and view) should address #180.4
https://www.drupal.org/node/2844297 is a CR
Opened #2844302: Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase for #184. I figure the discussion can happen on that issue? Adding to parent.
Comment #186
xjmI confirmed that #175 is fixed.
All the release management things seem to be squared away, so this is xjm, signing off!
Comment #187
tim.plunkettThe last RTBC patch was in #127, here's an interdiff from that point.
Patch still applied the same, but rebased anyway just in case.
Comment #188
jibranThe all new changes look good. Nothing related to functionality. Just some suggestion and a question. Setting it to RTBC beuase it is ready.
Is there a follow-up docs issue to write https://www.drupal.org/documentation/modules/field_layout page?
These should be together.
Let's keep the symmetry.
Stray @todo.
Comment #189
tim.plunkett#188
1) This is explicitly mentioned as a "Must Have" in the parent issue (#2795833: [plan] Add layouts to entity displays (both form and view)) but is not part of this issue
2) Fixed
3) Fixed
4) Fixed
Two docs changes, one code cut/paste, so leaving RTBC.
Thanks for the review!
Comment #190
tim.plunkettHad an interesting discussion with @effulgentsia today, specifically about ways the form portion of this could conflict with unknowns.
First, if you go through the Field UI and add a field called "Layout", the generated machine name will be
field_layout
.FieldLayoutBuilder::buildForm() then overwrites the field completely.
The workaround is to use
$build['_field_layout']
to store our additions to the form.Second, any alterations to the form structure using #group will cause conflicts.
The easy example is the node form. \Drupal\node\NodeForm::form() uses #group to split the node form into two columns.
We agreed that if a field had an explicit #group set, Field Layout should leave it untouched.
Once Field Layout is stable, we can remove core's usage of #group for entity forms, and use layouts directly.
I've opened #2845425: Replace hook_form_node_form_alter() implementations with configured field layouts to deal with this long-term.
Test coverage is added for both changes, and leaving at RTBC since it was discussed and approved in real-time, and because @effulgentsia indicated he's not finished his review yet anyway.
Comment #191
tim.plunkett@effulgentsia noticed that aggregator_entity_extra_field_info() defines extra fields that have the same name as non-configurable base fields as defined by Feed::baseFieldDefinitions() and Item::baseFieldDefinitions()
This is because those fields do not have formatters available, so the field definitions cannot be marked as configurable. But extra fields are always configurable.
Therefore our logic in FieldLayoutBuilder::getFields() needs to change. I did that, and added coverage for it.
@effulgentsia was also concerned about the direct array manipulation in FieldLayoutBuilder::buildView(), which isn't needed in ::buildForm() due to #groups.
I opened #2846393: [PP-1] Investigate alternative approaches to moving fields in FieldLayoutBuilder::buildView(), added it as a "Should Have" to the parent issue, and included an explicit @todo.
Also from our discussion I opened #2846389: Ajax progress indicator from layout settings is confusing, also adding it as a "Should Have" to the parent issue.
Finally, I made a dedicated issue for @swentel's point in #16 about the module weight: #2846395: Increase module weight of field_layout
That is a "Could Have" on the parent issue.
Comment #192
effulgentsia CreditAttribution: effulgentsia at Acquia commented#191 addresses all remaining framework manager concerns I could think of, so removing that tag.
@tim.plunkett is implementing a couple more minor things I pinged him about, so holding off on commit until then.
If anyone else has any remaining concerns about this patch, now is a good time to mention them. Otherwise, I expect to commit this within the next 24 hours.
Comment #193
tim.plunkettThere are 3 changes in this patch.
First, I reorganized the one test. Before it would set up a giant $expected array mapping to the entire display entity, for every step.
After time I found it hard to tell what was changing.
So now it directly manipulates the same array each time, with comments, so you can clearly see what is happening.
I split that out to its own interdiff.
Second, in #181 I halfway introduced a new logic branch but didn't finish it.
When switching layouts we have to remap the regions for each field. Before, it would blindly overwrite everything with the default region of the new layout.
Now if the old layout and new layout share a region name, the field stays put. This finishes that logic, and adds coverage for it.
Thirdly, FieldLayoutEntityDisplayTrait::preSave() was written to mimic ConfigEntityBase::preSave().
It ensures that the configuration was set on the plugin, and the plugin was consulted for updated values (allowing for things like default configuration).
This code would go away in #2844302: Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase, since it's only needed here because these are third-party settings.
Similarly, we have a ::calculateDependencies().
We don't need a __sleep() as we don't store the instantiated plugin as a property.
Finally there is ConfigEntityBase::set(), which we don't have a version of.
This adds code to setLayoutId() to approximate that, and adds test coverage.
The second and third change are in the main interdiff.
Since these were discussed with @effulgentsia, once again leaving RTBC.
Comment #195
tim.plunkettRandom failure in outside_in, retested and all is well.
CR was written already
Comment #196
effulgentsia CreditAttribution: effulgentsia at Acquia commentedTicking credit boxes for everyone! Thank you all for your insights that helped this issue.
Comment #198
effulgentsia CreditAttribution: effulgentsia at Acquia commentedPushed to 8.3.x and published the CR.
Comment #199
effulgentsia CreditAttribution: effulgentsia at Acquia commentedComment #200
jibranYay! thanks @effulgentsia.
Comment #201
Gábor HojtsyUpdated https://www.drupal.org/core/experimental#versions with this new module :)
Adding to #2846830: Add changelog for Drupal 8.3.0 now.
Comment #202
xjmGood catch!
Thanks @Gábor Hojtsy for updating the experimental module list. Looks good to me.