Add swappable (dynamic) grid systems to core

First patch.

@mortendk: well, the point of the plugin system is whatever grid support we add to core it will be infinitely extensible in core. Ie. if you ask the plugin system for grids supported, you'd get ones provided by any core plugins the same way you'd get them from a contrib plugin.

@effulgentsia: thanks for the review! I think it makes more sense indeed to wrap plugins into config entities then the other way around (like with the current patch), IF we consider grids would be config first and foremost. For layouts for example, the committed solution is that layouts are plugins but not configuration, so if someone wants to make editable layouts, there is no other way but to wrap plugins around config there. So you are proposing we approach this one the other way around.

That also obscures the storage of the data, since the CMI object would be a very limited shell for storing arbitrary plugin data, ie. in effect the config would have a $data key storing whatever structure the plugin needs and the config would merely be responsible for channeling that data to CMI. The plugin would need to maintain the data structure, the forms, etc. Given that config entities have a rich form handling flow, not sure what would be the best practice for tying the plugin form components to the configentity then?

In short, sounds like we could not use much of the data structure or form handling features of configentities and rather use them as a very light container just so we can use config entity lists and maintain the grids with CMI regardless of their plugin implementation. So the plugin would need to provide forms, UIs, and data structures?

Is this a fair summary?

@dasjo: indeed, the discussion focuses on making things pluggable exactly because *right now* some people prefer to build sites on a GUI and others prefer ruby/compass/Sass/LESS tools. Drupal 8 is going to be out for quite a few years (with the current support model it would be out and supported until Drupal 10 is released, so for at least 5-6 years from now based on the current pace of development). Who can tell what is going to be best practice in 4-5 years (when Drupal 9 might not yet take over as the primary platform used)? The reason we do focus on the lower level discussion of making it pluggable is so that contrib can help apply best practices as they evolve through the years.

I think your observation is spot on that the focus here is site-builders and not those knowledgeable themers who want to hardcode their layouts with up to date best practices at all times and have full control over their full CSS. This would not be a mandatory feature, and indeed used primarily for editable layouts, which is also not a target for precision themers. However, we would love the feedback to still have this feature as best as possible with the constraints it needs to be in.

Discussed this with Tim Plunkett in IRC as well:

[6:12pm] GaborHojtsy: timplunkett: hey
[6:12pm] timplunkett: GaborHojtsy: hi
[6:13pm] GaborHojtsy: timplunkett: so in http://drupal.org/node/1816650#comment-6620164 we want to introduce a new thing in core that is pluggable (uses plugins) and its default implementation (and possibly but not necessarily all assumed contrib interfaces) would use config for storage of backend data
[6:13pm] Druplicon: http://drupal.org/node/1816650 => Add swappable (dynamic) grid systems to core => Drupal core, other, normal, needs review, 8 comments, 5 IRC mentions
[6:13pm] GaborHojtsy: timplunkett: the question is how would we ideally combine plugins and config entities
[6:13pm] GaborHojtsy: timplunkett: which is probably very much at the heart of Views for D8 as well
[6:13pm] timplunkett: oh wow
[6:14pm] timplunkett: GaborHojtsy: well no, our configentity references several plugin types
[6:14pm] timplunkett: GaborHojtsy: but you can't swap out the config entity for something else
[6:14pm] GaborHojtsy: timplunkett: yeah, that possibly makes sense, but then your config entity cannot really make use of its form controller, etc. or it would need to relay al that to the plugin
[6:15pm] timplunkett: GaborHojtsy: rewriting the Views UI as a form controller is something i hope to work on at badcamp...
[6:15pm] GaborHojtsy: timplunkett: so you just use the config entity as a broker to store config data and plugins to provide the real behaviour logic / data formats, etc?
[6:16pm] GaborHojtsy: timplunkett: that is a config entity with one property $data, or somesuch
[6:16pm] timplunkett: GaborHojtsy: correct. think views exports: http://drupalcode.org/project/views.git/blob/refs/heads/8.x-3.x:/config/...
[6:17pm] timplunkett: GaborHojtsy: each time you see "id:" in there, its a plugin ID
[6:17pm] GaborHojtsy: timplunkett: ok, that makes sense, thanks!
[6:17pm] timplunkett: GaborHojtsy: just a side note for your patch, can you not add a procedural wrapper for the manager? just use drupal_container directly
[6:18pm] timplunkett: it'll save us clean-up later
[6:18pm] GaborHojtsy: timplunkett: yeah, I just took it from the layout stuff, but I can definitely take it out
[6:18pm] GaborHojtsy: timplunkett: thanks!

In short wrapping plugins in CMI objects is what Tim suggests (and what Views uses) for this type of problem as well.

Ok, took some drastic measures to help this get accepted:

- cut out everything related to a user interface (I did leave in one debug function to show you it works)
- made config instantiate plugins instead of the other way around
- made and proved the CSS generation actually works now (so feel free to bring your pitchforks and tear it apart, it is certainly not up to the standards we want, but that is the greatness of the community and collective knowledge :)

So you define a grid like this in .yml:

A six column fluid grid:

id: six_column_fluid
label: Six column fluid
pluginId: equal_column_grid
pluginConfiguration:
  type: 'fluid'
  width: 100
  columns: 6
  padding_width: 1.5
  gutter_width: 2

A 12 column 960px fixed grid:

id: ninesixty_12
label: '960px wide, 12 column grid'
pluginId: equal_column_grid
pluginConfiguration:
  type: 'fixed'
  width: 960
  columns: 12
  padding_width: 20
  gutter_width: 10

The grids are fully plugin based so you give the plugin id and the plugin configuration here, and the plugin is instantiated later using those. The system is fully pluggable, so none of the grids are required to have equal column widths or have a concept of fixed/fluid or have gutters and padding. This might be dangerous for the usefulness of grids (eg. we are not even sure a number of columns is represented in the plugin configuration which would be needed in some way for the editable layout efforts). We can rectify this by introducing a getNumberOfColumns() method on the plugin or somesuch that would compute/retrieve/return the number of columns, etc. So this does not necessarily need to be a limitation on the config.

You can load the grids using regular entity functions:

  $grid1 = entity_load('grid', 'six_column_fluid');
  $grid2 = entity_load('grid', 'ninesixty_12');

These will load the entity with the above configuration but will not yet instantiate the plugin for them, until you actually use it. You can get a plugin by calling $grid->getPlugin() or directly render the CSS via the grid object (which will use the plugin to render the CSS):

  $plugin = $grid1->getPlugin();
  // .. do something with $plugin
  // OR render CSS via the plugin (but you don't need to know its a plugin :)
  return $grid->getGridCss();

Any UI for this would need to use the plugin system as well to render the forms and validate/process the data. I'm not confident we can get a UI in in the first patch, so just proposing this simpler backend only implementation for now (like breakpoints in core).

We do definitely need tests for this, the included gridbuilder_help() debug function helps you see it actually works, and generates these CSS snippets respectively for the two grids:

.grid-six_column_fluid .grid-col { border: 0px solid rgba(0,0,0,0); float: left; -webkit-box-sizing: border-box; -moz-box-sizing: border-box; box-sizing: border-box; -moz-background-clip: padding-box; -webkit-background-clip: padding-box; background-clip: padding-box; margin-left: 2%; padding: 0 1.5%; } .grid-six_column_fluid .grid-col.grid-col_first { margin-left: 0; clear: both; } .grid-six_column_fluid .grid-col_1 { width: 15%; } .grid-six_column_fluid .grid-col_2 { width: 32%; } .grid-six_column_fluid .grid-col_3 { width: 49%; } .grid-six_column_fluid .grid-col_4 { width: 66%; } .grid-six_column_fluid .grid-col_5 { width: 83%; } .grid-six_column_fluid .grid-col_6 { width: 100%; margin-left: 0; }

.grid-ninesixty_12 .grid-col { border: 0px solid rgba(0,0,0,0); float: left; -webkit-box-sizing: border-box; -moz-box-sizing: border-box; box-sizing: border-box; -moz-background-clip: padding-box; -webkit-background-clip: padding-box; background-clip: padding-box; margin-left: 10px; padding: 0 20px; } .grid-ninesixty_12 .grid-col.grid-col_first { margin-left: 0; clear: both; } .grid-ninesixty_12 .grid-col_1 { width: 70.833333333333px; } .grid-ninesixty_12 .grid-col_2 { width: 151.66666666667px; } .grid-ninesixty_12 .grid-col_3 { width: 232.5px; } .grid-ninesixty_12 .grid-col_4 { width: 313.33333333333px; } .grid-ninesixty_12 .grid-col_5 { width: 394.16666666667px; } .grid-ninesixty_12 .grid-col_6 { width: 475px; } .grid-ninesixty_12 .grid-col_7 { width: 555.83333333333px; } .grid-ninesixty_12 .grid-col_8 { width: 636.66666666667px; } .grid-ninesixty_12 .grid-col_9 { width: 717.5px; } .grid-ninesixty_12 .grid-col_10 { width: 798.33333333333px; } .grid-ninesixty_12 .grid-col_11 { width: 879.16666666667px; } .grid-ninesixty_12 .grid-col_12 { width: 960px; margin-left: 0; }

The selectors are pretty flexibly configurable, these are just the defaults to scope the CSS such that multiple grid systems can be used on the page for different things (eg. for different breakpoints :).

Hopefully the scaled back scope of this issue helps focus on the real problems and the discussion to be on the base functionality :)

Not posting an interdiff, since this is pretty much a rewrite.

Fix minor CSS rendering bug. Any feedback on this approach (summary in above comment)?

I think these are good suggestions. For why it is not a GridInterface and grid.module, etc. there is http://drupal.org/project/grid existing, which is about input fields in a grid. Granted it did not have a D7 release and looks pretty much abandoned for years, so probably not an issue if we take over that namespace in core. Will work on an updated patch later today.

- renamed pluginId to type as suggested
- renamed pluginConfiguration to options as suggested (I think these two obscure how it works a bit, but I'm not crazy about it either way)
- equal_column_grid renamed to equal_column

- @todo: I kept type fluid/fixed in options for now; I don't think its misleading, although I see if we use 'type' for plugin identification purposes, then this might be misleading :) - I think it makes sense for it to be an option on the equal column grid; it might be best to collapse with the width key; other's feedback welcome!

- renamed everything from gridbuilder => grid, GridBuilder => Grid (eg. GridInterface, GridBundle, etc.)
- made Grid implement GridInterface and accept and forward the interface parameters proper
- removed the $defaults class key
- made all suggested changes to the EqualColumn annotation and removed its useless constructor
- renamed the module to grid.module and removed the breakpoint requirement that is not anymore valid

I think this resolves all suggestions but the fluid/fixed. It still works the same way as before but I agree is much more clean :)

Found one typo in the three column grid, and the Grid class properties were not properly renamed.

Time to drop debugging code. :)

@effulgentsia:

1. I don't see how reworking the EqualColumn constructor to take argument like this works as a swappable plugin. Other plugins could easily not have a fixed column number or have no gutters and paddings. You essentially dropped the implementation of PluginBase from the equal column grid.

2. URLs, was indeed a leftover, but you already removed it :)

3. t('Grid') is implemented now.

4. Type => Units, made that change.

5.

Why have wrapper_selector and col_selector_prefix as parameters of this method? Would they make more sense as config options, or are there use cases where the same grid entity needs to have its CSS rendered with different selectors?

Definitely NOT config options (as in options for the .yml files). We need to be able to render the same CSS with different selectors based on the use case we apply them. Eg. if we build a responsive page where different grids apply at different breakpoints, we'd need to apply different selectors per grid, we cannot just use the same selectors for all grids, we need to vary them. The responsive layout module uses this functionality to apply different selectors.

6.

Do we need 'grid-col-first' as a separate class from 'grid-col-1'?

and also

It's so weird to me that each column's width includes the width of previous columns. Then again, I'm confused by all sorts of stuff in CSS-land.

Well, grid-col-first is a class to apply to the first content in row regardless of its width, and grid-col-1 is a 1 wide column. Similarly, the reason for each column to be accumulating all previous columns is because grid-col-5 means its a 5 column wide width element, while grid-col-3 is just 3 column wide.

7.

Also, why do we hard-code the general .grid-col class despite allowing the prefix for the specific classes to be passed in?

We can rework this making any class overridable. What is your suggestion?

8.

We never (except for a couple exceptions in settings.php) have configuration in Drupal core for which we don't provide a UI.

That is not entirely true. We definitely have breakpoints that have no UI.

9.

However, while I think the EqualColumn plugin should stay, I think we should remove the 5 specific configurations (960/12, 960/16, fluid/3, fluid/6, and fluid/12) from this patch. We have no use case for them yet. When we add something to core that needs a specific default grid to work with, then we can add that default at that time.

I don't think a grid module without reasonable default config looks good, but if you believe it should be started off fresh (unless other modules land needing grids, then well, ok). We do need some grids in a testing module then anyway (similarly to how layouts were introduced).

10.

The first part seems done. Anything in 'options' is forwarded to the plugin: each plugin can decide what options, including any custom ones, it wants.

How do you imagine this happening? This cycles back to my point in 1? You made the EqualColumn plugin not even a plugin implementation and not very generic at all with its specific constructor, so I'm not sure how this is envisioned.

11.

I don't understand the breakpoint comment: is it still relevant?

At least for the responsive layout module, we need breakpoints to be associated with grids. Breakpoints do not have 'extensible' metadata capabilities, but at least grids to some degree had (before your last patch). So what I did in #1822950: Add responsive layout builder to core is to tack on breakpoint associations in the grid options. Lacking extensibility in breakpoints and grids, we'd need to introduce yet another new object there which is a grid-breakpoint bridge relating the two together. I don't like that idea much, given we already have way too many moving parts/objects and this is a relatively minor relation problem. However we'd ideally have a reasonably nice way to include this kind of extension configuration with the grid. Just tacking on the data might not be reasonable, but building a whole extra plugin layer might be overkill.

I kept this needs work for 1 and 10 at least.

Here is a very quick and untested stab at an admin UI:

- added back menu items
- added back form controller
- added back grid.admin.inc
- made the form controller delegate entity prepare, form and submission to the plugin

And this latest one is the most interesting. I did not touch your grid instantiation changes, but those make the grid (plugin?) class not really aware of the plugin structure (how the options are stored), but the entity preparation, form and form validation does not have much of a way around it (and also cannot avoid that data to be stored in an array). So your changes for instantiation look even more odd with these in.

I need to a quick errand now, so just posting for debates. Might not pass :)

1. Due to #1763974: Convert entity type info into plugins the hook_entity_info() part needed to go to annotations and the move of that to a plugin class (using other plugins nonetheless) was required.
2. Also fixed things noticed by @mbrett5062 in #1813910: Add region module to Drupal core (for editable responsive layouts) in the similar base code (name of delete confirm form function, lack of hook_help, $items initialisation in hook_menu(), etc).
3. Fixed the parse errors in the form controller.

Also need to use the annotation classes in the plugin class file to make it work at all.

Found a couple more issues with how the form is constructed.

All right, got down to do some tests and uncovered some bugs.

- the delete page had a wrong callback path

- the form options sub-element was not #tree TRUE, but then again, the error setup would need to know about the wrapper structure or would need to get one layer up to register errors for it, so I decided to go for simple instead and just merge the plugin fields on the top level; there should always only be one plugin and it should know the default fields

In the tests:

- moved default grids to the test, no grids defined now in the module itself
- have tests for the default grids, deleting and editing those
- then the creation of a fluid and a fixed grid and the editing and deletion of one of them

I did not add coverage yet for the CSS generated since we did not get much commentary on that yet.

Any comments overall pro or contra?

Trivial change to centralize the test initialisation. Should keep this RTBC.

#32: grids-in-core-32.patch queued for re-testing.

Well, anyway, due to core things not happening below this layer that we wanted to build on, we just need to go and focus on those things and will pretty likely not have a responsive layout builder (and therefore this module) in core. Unless someone comes and want to run with all these. Start from #1813898: [META] Add editable responsive layouts to Drupal core.

Given feature freeze, this is not going to happen in Drupal 8. Watch http://drupal.org/project/gridbuilder for this purpose in Drupal 8. Moving this to Drupal 9.

Updated issue summary.