Allow themes to identify their breakpoints to Drupal

Patch applies cleanly and all breakpoints correspondent to what's defined inside the css.

I adapted breakpoints module to support this new syntax, mainly to see if it works (and it does ;-)), but there's one thing left: the picture module uses 'breakpoint groups' to do his work, so we at least need a basic API to handle that.

I created a new issue to split the module into two parts: #1776236: Split breakpoints into API and UI module

Hm. Dries will probably want to have a look at this. He has never been a fan over overloading .info files.

Alternative patch as proof-of-concept using yml files

using $theme_key.breakpoints.yml

I had to add config_install_default_config('theme', 'seven'); to the test otherwise the config wasn't loaded.

+++ b/core/themes/bartik/config/bartik.breakpoints.ymlundefined
@@ -0,0 +1,3 @@
\ No newline at end of file

Add newline at the end of your config files

Alex, I uploaded the patches because I heard the same as #3, I'll try the tests with other themes to see what's going on.

Personally the more I work with config, the more I like it.

It looks like the config ins't loaded by the test framework, I added theme_enable(array('seven')) to force it.

+++ b/core/themes/bartik/config/bartik.breakpoints.ymlundefined
@@ -0,0 +1,3 @@
+wide: 'all and (min-width: 851px)'
\ No newline at end of file

Still no newlines at the end of the config files

Personally, I could go with either .info or .yml. I think extensions should probably all be yml and we should ditch .info but thats for another issue. Anyway, lets let committers decide between effulgentsia's patch and attik's latest.

For me, the Test isn't worth the time it takes to run. It sets up a whole Drupal just to show that we can do a $config->get() [already tested elsewhere] and return an item out of its array. If this were a pure unit test of system_breakpoint_list(), then it would be very fast (but still pointless!). No idea how feasible a unit test is for this in today's simpletest.

Overall, the config file approach makes much more sense to me for this use-case.

In #1712250: Convert theme settings to configuration system and some related issues, we're considering to move some of the theme .info stuff into a [theme].settings configuration object. A [theme].breakpoints would nicely complement that.

So +1 for using config. :)

On the patch:

+++ b/core/modules/system/lib/Drupal/system/Tests/Common/BreakpointsTest.php
@@ -0,0 +1,36 @@
+ * Definition of Drupal\system\Tests\Common\BreakpointsTest.

Can we move this test into Drupal\system\Tests\Theme ?

+++ b/core/modules/system/system.module
@@ -3126,6 +3126,27 @@ function system_region_list($theme_key, $show = REGIONS_ALL) {
+function system_breakpoint_list($theme_key) {

I don't really understand why this function is part of system.module -- the configuration clearly seems to belong to a particular theme (not system module), so why don't we put it into theme.inc as

theme_get_breakpoints()

?

What do you think?

Nice. :) Unassigning from Dries as this makes this a lot less controversial.

Nice one.

Quick question: we expect module builders to know that there might be a config called $theme_key.breakpoints? Is a wrapper function not easier for developers?

They will be used to the config system. So I don't see a reason why it should be bad for dx.

With the code in #13 the breakpoints module could just call system_breakpoint_list($theme_key); to get a possible list of defined breakpoints by the theme, without a wrapper function I'll need to do something like

$theme_breakpoints = array();
$config = config($theme_key . '.breakpoints');
if ($config) {
   $theme_breakpoints = $config->get();
}

Meaning I will provide my own wrapper function, as well as probably any other module wanting to use this, hence the question.

This is the case for a lot of other config conversions. We can't write a wrapper for all of them. But yeah I can see contrib code dealing with breakpoints write a wrapper for it.

@aspilicious true, so RTBC for me as well

Does theme config actually get copied to the config directory when themes are enabled? My assumption would be that it doesn't yet. That notwithstanding I'm also much happier with this in config than .info.

I just checked, it doesn't get copied but it is accessible.

If it doesn't get copied, then it won't be available to config() no? If it's not I think we need to add that here, although not sure if that would introduce a dependency on #1067408: Themes do not have an installation status then?

There's something strange going on, I now have a config directory called "Array" and it contains bartik.breakpoints.yml

I did a full clean install with the patch from #16 and now the config contains bartik.breakpoints.yml, but not seven.breakpoints.yml.

I changed the admin theme to bartik, disabled seven, enabled seven and now seven.breakpoints.yml is also there.

So only problem is that the installer doesn't copy seven.breakpoints.yml if that needs to be fixed in this issue change to status, for now back to RTBC.

OK so standard_install() doesn't enable seven:

http://api.drupal.org/api/drupal/core%21profiles%21standard%21standard.i...

We should at least change that here I think.

Is there're a reason why it is enabled like this:

 // Enable the admin theme.
  db_update('system')
    ->fields(array('status' => 1))
    ->condition('type', 'theme')
    ->condition('name', 'seven')
    ->execute();

If not we can just do

theme_enable(array('seven'));

Back to RTBC, #30 is solved in #1712352: Configuration system does not support themes

An interesting related problem is declaring layouts from themes: #1787846: Themes should declare their layouts. Do you agree that CMI .yml files would be appropriate for that use case as well or something else should be used. This approach inspired me to propose that there. It would such if things use different technologies. Eg. regions are in the .info file, breakpoints are in .yml and then layouts would be yet another discovery system and/or file format. Feedback welcome there!

I think most anything that you're able to alter through a UI fits well with config. In my view, .Info files generally contain hard and final information that can't be changed without breaking the theme itself (like regions, required css and js files, etc.).

@RobW: as per the intent of this issue, these breakpoints are just a static copy-paste from the breakpoints already defind in CSS to inform the rest of the system.

Can this get committed or are there still some problems left?

We're over thresholds atm so I unfortunately can't commit any feature patches. :( Help with smashing critical and major bugs would be greatly appreciated.

~6 clicks. Apply patch > Commit > Push to remote > Password > Ok. Takes me half a minute. That's why nothing get's forward with core patches? Please grant me commit permission, I can help.

Hass, I think the threshold Webchick is talking about is the number of critical bugs in Drupal 8. If it's over a certain amount, no new features will be committed until the bugs are fixed. So in order to get this committed more quickly you would have to help fix the current Drupal 8 critical bugs.

See the "issue thresholds" section on drupal.org/node/1201874.

The wrong way is "wait until D8 is fixed" to get a properly working D7 core. Than D8 will be released later - who cares. We cannot use unready banana D7 and this is more important than future releases. We are in the situation that we are waiting e.g. a shiny SSL patch #961508: Dual http/https session cookies interfere with drupal_valid_token() or #1067120: Missing hook_taxonomy_term_view() or hook_entity_view() when viewing a term for D8 and this hold back to fix a totally broken D7 core. Great! And then we start with D9 and miss D7 and D8 again.

I have a very good understanding about regressions, but what we are doing here is plain wrong. We make D7 users waiting for over-designed patches for a future core version (that may also never get ready) and block them from implementing current (un-)stable D7 versions. Something with the focus is really wrong.

@hass I understand your problem, but polluting this issue isn't going to solve much, I think it's better to create a separate issue to discuss this.

Actually, even better is to just fix bugs, then this becomes a non-issue. :)

There are D7 real life bugfixes in queue, 2+ years RTBC, still no commit.

@hass, which of the 7 issues currently RTBC for 7.x has been there for 2 years?

Yeah, that sounds like a pretty egregious exaggeration.

In any case, we are now below thresholds, and this patch looks like a great first step to supporting responsive layouts and images in Drupal 8! I did find it confusing that these were added without an API or UI or something to use them. Alex informed me that these are going to be added in subsequent issues (e.g. picture element) and this is an unblocker. It looks like catch's concerns were addressed in #1712352: Configuration system does not support themes.

Therefore, committed and pushed to 8.x. Thanks!!

I'm pretty sure at this point we don't need a change notice, although at whatever spot we end up adding an API/UI to use these, we might.

Post-emptively tagging "Spark."