Problem/Motivation

At present, the only way to configure breakpoints is through config.yml files in a theme's config/install folder. However, those configurations can only be made in the breakpoint YML config files before a theme is installed.

Once a theme is installed, the only way to edit the breakpoints is to use configuration management to export the breakpoint config, edit the file, then import it.

If a theme has a pre-set layout that isn't going to change, and breakpoints have been set up to match how images will be used in that layout, that workflow might be marginally okay.

However, many people working with a site's theme start with a base theme that doesn't necessarily have a specific layout and associated breakpoints. Creating a theme design with layout and appropriate breakpoints for image usage within that theme is an iterative process. Try something with the layout, see if it looks okay, tweak it, check it, and retweak it. Having to export breakpoint config files, edit them and import them within that iterative process would be very cumbersome.

Because we're getting down to crunch time with Drupal 8, right now we aren't going to be able to implement a robust visual interface for configuring breakpoints, as envisioned in: #1701112: Advanced responsive images UI/UX for breakpoints. It's worth taking a look at that issue, as there are some valid comments, but what we need right now is a very simple UI for breakpoint configuration.

The Drupal 7 Breakpoints module do not visually demonstrate breakpoints, even though they do provide an interface to associate a set of media queries together into a group. https://drupal.org/project/breakpoints

While that would allow themers to enter the media queries for a breakpoint group, clone a breakpoint group, edit a breakpoint group, deleted a breakpoint group, etc (all that CRUD), the Drupal 7 breakpoint UI has proven difficult to use. As well, setting up any UI this late in the Drupal 8 cycle may be difficult.

Because we are implementing the current version of the picture element (picture.responsiveimages.org) and supporting that with the Picturefill 2.0 Polyfill (#2260061: Responsive image module does not support sizes/picture polyfill 2.2), which allows for the new sizes attribute, it is also important for sizes to be able to configured for breakpoints. That is currently being handled in the revised responsive images UI, so we don't need to handle that here. For reference, here is a good article that explains the srcset and sizes UI: http://ericportis.com/posts/2014/srcset-sizes/

It cannot be emphasized enough that without basic configuration abilities, it will be very, very difficult for anybody to set up responsive images on a Drupal 8 site.

Proposed resolution

Since a breakpoint UI is out, breakpoints must be configured through another means. Modules and themes can provide a EXTENSION_NAME.breakpoints.yml in their root directory. These are discovered using the plugin system and YAML discovery.

If at some point in the future there was a desire to make a UI for breakpoint groups (in 8.1 or 8.2 for example), it would be possible to implement a module that adds to the definitions using the hook migrate breakpoint settings from an info file to CMI.

Another benefit of this approach is that it would allow breakpoints to be edited in code, rather than in a UI. This is probably an easier workflow, as most media queries for breakpoints with CSS or SCSS files will likely be handled in code by themers as well.

Example breakpoints.yml file

stark.mobile:
  label: mobile
  mediaQuery: '(min-width: 0px)'
  weight: 0
  multipliers:
   - 1x
stark.narrow:
  label: narrow
  mediaQuery: 'all and (min-width: 480px) and (max-width: 959px)'
  weight: 1
  multipliers:
    - 1x
stark.wide:
  label: wide
  mediaQuery: 'all and (min-width: 960px)'
  weight: 2
  multipliers:
    - 1x

By default breakpoints are placed in a group with the same name as the provider - in the above case that would be 'stark'. Custom groups can be created by adding a group key to the breakpoint definition.

Remaining tasks

Review patch.

User interface changes

None.

API changes

  • Breakpoint module has been completely rewritten.
  • ResponsiveImageMapping configuration entity storage of mappings has been altered to avoid storing dots in configuration key names
  • In changing the ResponsiveImageMapping entity added new methods to make it easier to manage them
Files: 
CommentFileSizeAuthor
#137 interdiff-134-137.txt1.86 KBJelle_S
#137 2271529-translate.137.patch150.12 KBJelle_S
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,106 pass(es).
[ View ]

Comments

attiks’s picture

The UI for sizes is part of the mapping and already implemented in the patch.

sun’s picture

Various people mentioned that they'd rather like to have the breakpoint definitions in the theme's .info.yml file.

This would inherently cause them to be "theme info/settings" instead of configuration, and thus, they'd still be editable after a theme has been installed.

Based on that consideration, I think a breakpoint UI should be limited to be a simple "YAML code generator"; i.e.,

  1. it automatically loads the currently defined breakpoints from the theme's .info.yml file
  2. you can edit/tweak them in the UI…
  3. you click an "Export" button that simply shows a textarea from which you can copy the new definitions into your .info.yml file.

It's my impression that this is what >90% of all themers expect. — Especially the aspect of being able to change the breakpoint definitions after installation, as well as the aspect of ensuring total ownership for themes, because the definitions actually have to be resembled in the theme's CSS, too.

Bojhan’s picture

I am still not sure about this, I think I mentioned it in every Breakpoint UI discussion thus far. I don't really see why we have another one, but as always my argumentation:

1) The UI that we expose for Breakpoints, or earlier tries so far - have been incredibly difficult and complex for users to understand. This proposal seems to show no process or understanding, on how to resolve that.
2) The step for themers to do this in the theme its info.yml file is very small. It seems counter intuitive to me, for such a "programming" heavy task to be moved towards the UI. Tweaking happens in concurrence with CSS changes. Why would we force them to go to the UI for breakpoints?

At this stage in the release cycle (near the end), of polishing we should not be introducing major new UI's. Especially knowing it will inherently have many UX difficulties. It should be totally possible to introduce this in 8.1, if we take care of the breakpoints being in the info file step.

marcvangend’s picture

I wrote down some thoughts and concerns before in #2260061-7: Responsive image module does not support sizes/picture polyfill 2.2 and #2260061-13: Responsive image module does not support sizes/picture polyfill 2.2, but what it comes down to is this: I think we should not have a Breakpoints UI.

In the new picture element, the breakpoints listed in the sizes attribute should mirror your CSS’ breakpoints exactly (says Eric Portis). To achieve that, Drupal (read: the PHP involved in rendering picture elements) must either have knowledge about the theme's breakpoints, or must control the theme's breakpoints. So the question is: who is leading, and who is following? Should Drupal's breakpoint config follow the theme's CSS (Sass / Less / Stylus / whatever-comes-next) code, or should the CSS code follow the configuration set in the Breakpoints UI?

From a UX point of view, if Drupal offers a breakpoint UI users would expect it to control both the theme and responsive images (but that kills DX, see below). If the config from a breakpoint UI is used for responsive images only (like the D7 Breakpoints module), we'll have to tell users: "This is where you set breakpoints, but not really because it will not change your layout, and you're not allowed to touch this anyway because the settings must match the media queries of your theme." In other words, it would be a UI that does not produce the expected result, and makes it way too easy to get things wrong.

From a DX point of view, I strongly feel that themes should be leading, and themes should make their breakpoints configuration available to other modules that need the info. I have worked with a D7 base theme that tries to make UI-configured media queries available in the Sass code, and IMHO it was a nightmare. Front end devs should be free to use the front end tools of their choice without having to worry about integrating breakpoint config coming from Drupal. Themers who are skilled enough to write CSS or Sass, must surely be able to write down the media queries in the .info.yml file, thus exposing the info to Drupal modules.

mdrummond’s picture

After chatting with timplunkett and webchick on IRC to clarify a few things, I'm on board with putting these settings in the theme's info.yml file.

Making this change means we won't be using config entities for breakpoints anymore, so there could be a significant number of things to change, but I think it will be worth it.

Instead of using CMI to move breakpoint settings from a development server to a production server, the settings would be synced by moving the theme's info file, whether that's with git or FTP or whatever. When changes are made on either dev or prod, clear cache would be needed to get the settings change to apply.

I think this is a pretty decent workflow. If you're changing your site's layout, you're almost assuredly working with CSS files, and if you can change media queries there, changing them in an info.yml file isn't a big stretch.

If we go this route, I don't think any UI is necessary.

To marcvangend's point, yes, breakpoints for images are going to be connected with your layout's breakpoints, but as I've said before, there's no magic way to make this work. Whether the breakpoints are handled in a theme's info file or in a UI, you're ultimately manually putting a media query in a theme file as well as in the theme's css files. Although with a info.yml file, maybe there's some crazy way to use Grunt to automatically sync breakpoints between the two. Anyhow, it's not magic, it's manual either way. But the info.yml file feels "closer to the metal" to me, and probably a better solution.

And as timplunkett pointed out, if there's an idea for a great UI down the road, then it's easier to move from a theme settings file to config than the reverse.

So overall, I think the info.yml file route is the best way to go to make it easy for themes to change the breakpoint settings for a theme.

marcvangend’s picture

mdrummond: Completely agree. Move code + clear cache sounds like a good deployment workflow.

I do realize that matching breakpoint config to theme media queries is still the responsibility of a human being, capable of making mistakes. I'm not expecting magic of any kind. Our job is just to make it easy to get everything matched up (while a UI would only make it easy to get it wrong).

Slightly off-topic, but worth a mention: I just realized that the mapping of breakpoints to image styles will have to be configured per theme, just like the placement of blocks in regions is configured separately for each theme.

mdrummond’s picture

Good point about breakpoint configuration per theme. That's probably not a terrible idea anyhow, as there's a decent chance layouts will differ between different themes, although not necessarily. That might cascade over to responsive image mapping too, I'd imagine. For responsive image mapping, you might need to select the theme you're doing the mapping for, then configure each of the breakpoint groups that are set for that theme.

Does that sound workable, attiks?

attiks’s picture

I agree we don't need a UI for breakpoints, but not 100% sure about picture mappings per theme, especially since we now have support for sizes and you can do things like the following, which does not use any breakpoint at all. Besides that adding theme the a picture mapping will complicate the UI of picture mappings even more.

<picture>
    <source sizes="100vw" srcset="pic400.jpg 400w, pic800.jpg 800w, pic1600.jpg 1600w">
    <img src="pic400.jpg" alt="The president giving an award.">
  </picture>
attiks’s picture

StatusFileSize
new31.64 KB

Screenshot so you know what I'm talking about (keep in mind that the theme has to define an empty breakpoint)

The media query in the screenshot is not used in the theme, only for this mapping

mdrummond’s picture

That's pretty cool.

So just to think this through, if responsive image mappings aren't tied to a theme, then the responsive image mapping UI would just display the breakpoint groups defined in all of the themes, right? So in theory you could be configuring picture mappings on a breakpoint group for a theme that is enabled but not the default theme.

Do you think it might at least be possible in the responsive image mapping UI to indicate the applicable theme for each breakpoint group? Even if it's only on the UI for the settings for that particular breakpoint group's mapping?

attiks’s picture

Right, we can display their machine name or the theme they or defined in.

Keep in mind that modules can define breakpoints as well, the new nav module uses this, so we need a way to handle this as well, but now idea if this is possible with the proposal of @sun.

mdrummond’s picture

Could something similar be done with modules, putting breakpoint settings into a module's info.yml file? Not sure if that would be confusing trying to collect breakpoints from info.yml files from both themes and modules.

If that does work, same thing, would be nice to the machine name or the user-friendly module name that generates the breakpoint group.

mdrummond’s picture

Title:Basic UI required to configure breakpoints after theme installation» Move breakpoint settings to theme and module *.info.yml files

timplunkett indicated on IRC that we should be able to set breakpoints in a module's info.yml file as well.

mdrummond’s picture

Issue summary:View changes
xjm’s picture

Issue tags:+Configuration system

(Just so that we can find this issue from the CMI side.) :)

dcrocks’s picture

Issue summary:View changes
Issue tags:-Configuration system

Since it has been brought up here the relationship between both breakpoint definitions as well as responsive image mapping definitions and their source needs to be cleared up. A mapping can be created by a module, a theme, and through the UI. A breakpoint can only be created by a module or a theme. Though you can change a mapping's breakpoint selection, you can not delete a mapping once created. A breakpoint definition is deleted when its source is disabled. Since a mapping can point to any breakpoint from any source, what do you do if the currently active mapping has its breakpoint definitions deleted? Or if the breakpoint definition were changed by an update to the module or theme that created it? It just seems there is something basically wrong here abut how responsive images consume breakpoints.

attiks’s picture

#16:

1/ we need to allow a user to delete a mapping
2/ if you delete a breakpoint definition that is still used by a mapping, thing s will probably break, but I guess that's similar for other components as well. If you disable a link field while it is used in a content type and/or in a view it will break as well.

mdrummond’s picture

Issue summary:View changes
Issue tags:+Configuration system

Just a friendly reminder not to ignore a warning that an issue has been edited when you make a comment. Doing so can mean reverting an issue summary, issue name change, tag additions/deletions, etc.

I was able to go back in browser history to find the revised issue summary so I could repost it thankfully.

dcrocks’s picture

#18 Sorry about the override.

I created an issue for this, #2203459: responsive_image mapping doesn't implement 'delete' function, but it is more than allowing a delete operation in the UI. It needs to be discussed(over there) as to what to do if a module or theme is disabled.

sun’s picture

Issue tags:+beta blocker
  1. I've the impression we've reached agreement on how things are supposed to work.

    At the same time,

  2. This discussion apparently revealed a huge can of worms encompassing a dozen of architectural flaws in the current system.

    Therefore, I wonder whether

  3. This issue should be promoted to a beta blocker.

To clarify that can of worms:

  1. Some other stuff ([responsive] image styles, picture, etc) is apparently tied to breakpoints (currently config), but breakpoints should be definitions. Dependency (type) error.

  2. Breakpoints must be defined per-theme, not globally, because it's impossible to "force" breakpoints onto a(ny) theme.

  3. D8 seemingly even introduced a highly questionable facility of modules being able to define breakpoints, completely detached from any knowledge of any theme.

    Such a 3D model - in which two dimensions are hard-coded in independent CSS files - is impossible to work out in practice.

  4. ...

The sum of these aspects is not just one or two independent bugs — it's a series of functionalities that has been tacked onto an architecture that is missing a fundamental key constraint/filter: Nothing is global, everything MUST be restricted to a particular theme.

xjm’s picture

This is not a critical issue (nor should it be) and therefore does not block a beta release. Also, a reminder that the beta blocker tag should only be added by a core maintainer.

Switching to "beta deadline" which is the correct tag here.

xjm’s picture

Issue tags:+beta target

And beta target for visibility, because the breakpoint configuration implementation is something that would we should fix if we have time.

mdrummond’s picture

I'm thinking about this some more, and I think it's really odd that we're defining the sizes attribute in the picture mapping UI and associating that with an empty breakpoint.

Sizes and srcset is a good choice when responsive images are varying based on viewport sizes.

Setting up a breakpoint group with a separate media query per source element really only makes sense if you're doing art direction: defining different cropping per media query so that smaller viewports can focus more directly on the subject of the image for example.

It's possible that there might be some good tools developed for art direction down the road, but right now, those don't exist. I would guess for 95% of people trying to use responsive images, they're going to want to use the sizes and srcset attributes.

So if the goal is to have the definitions in the theme settings, then it makes a lot more sense to define sizes there, since that's a lot closer to where the layout and the CSS for images is being defined. Just to recap, the sizes attribute lets you define a set of media queries, and the width of the containing element for the image at each of those breakpoint, typically as a percentage of the overall viewport size. Then you can define the set of image files (and the width of those images) in the associated srcset attribute, and the browser gets to make the determination of which image file makes the most sense to use at any given breakpoint, based on whether the image is a good fit for the containing element, taking into consideration the resolution of the device and possibly the available bandwidth.

Defining the set of images to go into srcset alongside sizes makes sense to do in the responsive image mapping UI, which is already set up to select a set of image styles. But I'm not as sure about defining sizes on the mapping page, particularly if it requires setting up an empty breakpoint in the theme settings file. I'd guess the vast majority of themes would just end up having this empty breakpoint set up in the theme settings file, possibly multiple times to account for the different use cases for images on the site.

If there are going to be settings in the theme file, what you'd probably want is to be able define multiple image use cases (aka breakpoint groups). For each, there could be a toggle for a sizes-based use case or an art direction-based use case. (I guess in theory you could have multiple source elements with a sizes attribute on each, probably if you set a different image file type on each use case.)

I don't know, point being I'd suggest defining the sizes attribute in the theme settings, rather than the responsive image mappings UI. It certainly could work in the mapping UI, I just think it's going to end up kind of weird for most theme settings.

As for sun's questions:

1) I'm not clear on what you mean by the dependency type error. Yes, the responsive image module depends on breakpoints, and responsive images currently reads from config: that would definitely need to be changed to read the breakpoints from the theme settings file instead. Is that what you mean?

2) Are you saying it's a good thing or a bad thing to define breakpoints per theme? My thought is that probably makes a little more sense than defining breakpoints globally, since breakpoints are likely to be tied pretty tightly to a theme's layout.

3) It is a little bit odd for modules to define breakpoints, but I can see that making sense from time to time. The responsive toolbar defines its own breakpoints, for example, and since that sits above the rest of the layout, it's more than fine for it to change from a compact mobile layout to a wider layout wherever the module developer thinks makes sense. I'm not sure there are going to be that many use cases of that, but I think it would be more problematic if each theme needed to define its own set of breakpoints for the responsive toolbar.

4) Again, I'm not clear sun if you think it's a good thing or a bad thing to have breakpoints be dependent on a theme. Could you please clarify?

sun’s picture

Yes, I meant pretty much everything the way you described it.

I'd just like to use clear terminology: Breakpoints should be defined by a theme. The theme should be the owner, controller, and originating source. Breakpoints should not be configuration that is "dependent" on a theme; breakpoints should be definitions of a theme, because breakpoints are hard-coded in the asset files of the theme.

This implicitly means that breakpoints are no longer configured globally, independent from a theme. Instead, they are explicitly defined by a theme. If a theme defines no breakpoints, then there are no breakpoints.

Since breakpoints are no longer configuration, but definitions in code, their maintenance will need to change. The most sensible approach I can think of is to (1) collect all definitions, (2) store all current definitions in a state record, and (3) if a later rebuild identifies that definitions were added/changed/removed, then corresponding hooks/events are invoked, so as to allow other modules to react accordingly (e.g., responsive_image).

mdrummond’s picture

Since this is tagged with beta deadline, we need to find ways to move forward on this rather quickly.

Having breakpoints defined in a theme seems to make sense. The question is if modules can also define breakpoints. I'm guessing you'd say, no, sun, but if not, then how do we handle the breakpoints for something like the responsive toolbar?

The responsive toolbar's breakpoints don't necessarily have anything to do with a theme's layout, and expecting each and every theme to define separate breakpoints for the responsive toolbar seems like a bad idea. The admin experience, which the responsive toolbar is a part of, should be consistent no matter which theme somebody is using for a site.

As for collecting definitions, storing them in a state record, and invoking hooks on CRUD actions, are there already ways to do that sort of thing? Or is it something we'd have to create from scratch? If the latter, how complicated would it be to do so? Is there any way you could help with that, sun?

I think we also need to address the concerns I raised with the appropriate location for defining sizes. Personally I think that would make most sense in the theme's info file, but I think we need feedback from attiks or jelle_s on that.

Jelle_S’s picture

Status:Active» Needs review
StatusFileSize
new107.41 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,533 pass(es).
[ View ]

This patch moves from config entities to \Drupal::states()

Works, but could probably use some tests:

  • Info files are checked when the breakpoint module is enabled
  • Info files of modules that get enabled afterwards are checked when they get enabled
  • Info files are re-checked on cache clear
Jelle_S’s picture

Above patch does not yet implement #24-3 (hooks/events when breakpoints are added/changed/deleted)

LewisNyman’s picture

Issue tags:+frontend
mdrummond’s picture

I'm letting go of my concerns about where sizes should be. We have a workable solution with this that lets people edit breakpoints and sizes after a theme is installed. That's a huge improvement, and we should try to get that in since this has a beta deadline.

Looks like what we have left is the hooks/events for when breakpoints are added/changed/deleted.

Anything else we need to get this committed? Who needs to review? Additional tests needed? Profiling?

Jelle_S’s picture

Assigned:Unassigned» Jelle_S

I'll start working on implementing the insert/update/delete hooks

Jelle_S’s picture

Assigned:Jelle_S» Unassigned
StatusFileSize
new107.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,655 pass(es).
[ View ]

Did some manual testing as well to make sure everything worked after rerolling this patch because of the PSR-4 commit.
Hooks are in place, breakpoint.api.php added to document the hooks.

attiks’s picture

  1. +++ b/core/modules/breakpoint/breakpoint.module
    @@ -62,11 +71,185 @@ function breakpoint_group_select_options() {
    +/**
    + * Implements hook_themes_enabled()
    + */

    typo, is hook_themes_disabled

  2. +++ b/core/modules/breakpoint/tests/themes/breakpoint_test_theme/breakpoint_test_theme.info.yml
    @@ -4,3 +4,52 @@ description: 'Test theme for breakpoint.'
    +  theme.breakpoint_test_theme.mobile:
    +    id: theme.breakpoint_test_theme.mobile
    +    name: mobile
    +    label: mobile
    +    mediaQuery: '(min-width: 0px)'
    +    source: breakpoint_test_theme
    +    sourceType: theme
    +    weight: 0
    +    multipliers:
    +      1x: 1x
    +    status: true
    +    langcode: en

    have a look if we can simplify this, I guess we only need name, label, mediaQuery, weight and multipliers. Since breakpoints can not change groups anymore (they are tied to a theme or module), it should be possible to load all of them at once.

  3. +++ b/core/modules/responsive_image/responsive_image.module
    @@ -200,7 +199,7 @@ function theme_responsive_image($variables) {
    +    $breakpoint = breakpoint_load_breakpoint($breakpoint_name);

    if we simplify the config, we need to load all breakpoints of a group.

Jelle_S’s picture

StatusFileSize
new111.91 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,963 pass(es).
[ View ]

new patch with comments of #32 addressed.

xjm’s picture

Since this is a beta deadline, I think that precludes the need to revisit before release. :) Adding a reference to the "revisit before beta" issue to remove breakpoint from core.

mdrummond’s picture

I've been trying to ask sun to review this, as it was suggested he might be a good person to take a look (and he had suggestions about this earlier), but I haven't heard back from him. Does anybody have any thoughts on somebody else that could maybe review this?

Cottser’s picture

Assigned:Unassigned» Cottser

I can give this a review over the next few days.

attiks’s picture

@Cottser thanks

Cottser’s picture

Assigned:Cottser» Unassigned
Status:Needs review» Needs work
Issue tags:+Needs reroll

This needs a reroll, I should have checked yesterday.

Cottser’s picture

Reviewing the latest patch initially and reading through the issue everything makes sense so far, and this is removing a lot of code and still seems to keep quite a good amount of test coverage.

Some minor points below.

  1. +++ b/core/modules/breakpoint/breakpoint.module
    @@ -46,10 +43,22 @@ function breakpoint_help($route_name, Request $request) {
    +    $group_id = $module->getType() . '.' . $module->getName() . '.breakpointgroup';
    ...
    +    $group_id = $theme->getType() . '.' . $theme->getName() . '.breakpointgroup';

    @@ -62,11 +71,162 @@ function breakpoint_group_select_options() {
    +    $breakpoint_group = \Drupal::state()->get($module->getType() . '.' . $module->getName() . '.breakpointgroup');
    ...
    +    $breakpoint_group = \Drupal::state()->get($theme->getType() . '.' . $theme->getName() . '.breakpointgroup');
    ...
    +      'id' => $extension->getType() . '.' . $extension->getName() . '.breakpointgroup',
    ...
    +  $breakpoint_group = breakpoint_load_breakpoint_group($extension->getType() . '.' . $extension->getName() . '.breakpointgroup');

    I wonder whether we should be considering some kind of helper function for these, similar to what Drupal\breakpoint\Entity\BreakpointGroup::id() was doing.

  2. +++ b/core/modules/breakpoint/breakpoint.module
    @@ -62,11 +71,162 @@ function breakpoint_group_select_options() {
    + * Save breakpoints from an extension (module or theme).
    ...
    + * Delete breakpoints from an extension (module or theme).
    ...
    + * Load a breakpoint group.
    ...
    + * Read breakpoint groups from all extensions.
    ...
    + * Remove all breakpoint groups.

    Minor: I think the first word in the summary lines of these should be plural per https://www.drupal.org/node/1354#functions

Jelle_S’s picture

Status:Needs work» Needs review
StatusFileSize
new112.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch i2271529-40-breakpoints-info-file.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new5.68 KB

Thank you for the review!

Here's a reroll with the points from #39 addressed.

xjm’s picture

Priority:Major» Critical
Issue tags:-beta deadline

Discussed with @alexpott. In his assessment the current implementation is not releasable, but the upgrade path is doable after beta since we would simply no longer be using CMI for this. So it's just an API change for things that implement breakpoints plus a little cleanup deleting the config entities.

attiks’s picture

@xjm Do you know if Alex's concerns are with this patch or with the implementation of picture?

mdrummond’s picture

I checked with Alex, and he's in favor of the general direction of moving from configuration entities to the theme .info.yml file. He didn't have time to fully review this, as he's working on beta blockers.

So now that this is critical, hopefully we get some more eyes on this and can take care of any potential issues, then get this in.

catch’s picture

Issue tags:+D8 upgrade path
mtift’s picture

You can read more about Alex's thoughts on the breakpoint module in #2120875: Remove breakpoint and picture module from core. I can't really speak for Alex, but last week he said he would be fine with this patch as an alternative to removing breakpoint from core.

attiks’s picture

@mtift thanks for the feedback, this is a much cleaner solutions, so I'll hope we can quickly move forward

Status:Needs review» Needs work

The last submitted patch, 40: i2271529-40-breakpoints-info-file.patch, failed testing.

Jelle_S’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,283 pass(es).
[ View ]

Reroll of #40

Jelle_S’s picture

Issue tags:-Needs reroll
Jelle_S’s picture

StatusFileSize
new112.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,554 pass(es).
[ View ]

Maybe an actual patch would help... (Oops)

mdrummond’s picture

Came across this issue related to theme settings architecture. We should determine if some of the items in here might affect this issue. #2235901: Remove custom theme settings from *.info.yml

mdrummond’s picture

After reading through that issue, it looks like it's specifically tied to theme settings. I don't think that would necessarily apply to breakpoints defined in the info.yml file, which seems sort of similar to regions being defined in the info.yml file.

mdrummond’s picture

  1. +++ b/core/modules/breakpoint/breakpoint.module
    function breakpoint_group_select_options() {
    ...
    +    $group_id = breakpoint_breakpoint_group_id($module);
    +    $breakpoint_group = \Drupal::state()->get($group_id);
    ...
    function breakpoint_select_options() {
    ...
    +    $breakpoint_group = \Drupal::state()->get(breakpoint_breakpoint_group_id($module));

    These two functions both retrieve breakpoint groups. However, the first stores the id in $group_id, while the second does not. I think storing in $group_id is a little easier to read.

  2. +++ b/core/modules/breakpoint/breakpoint.module
    function breakpoint_group_select_options() {
    ...
    +    $group_id = breakpoint_breakpoint_group_id($theme);
    +    $breakpoint_group = \Drupal::state()->get($group_id);
    ...
    function breakpoint_select_options() {
    ...
    +    $breakpoint_group = \Drupal::state()->get(breakpoint_breakpoint_group_id($theme));

    The same is true for retrieving the breakpoint groups for themes.

YesCT’s picture

StatusFileSize
new110.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/modules/responsive_image/responsive_image.module.
[ View ]

reroll
for
#697760: Replace getInfo() in tests with native phpDoc + annotations (following PHPUnit)
which changed tests that this patch was removing.

just removed the tests.

Status:Needs review» Needs work

The last submitted patch, 55: i2271529-54-breakpoints-info-file.patch, failed testing.

YesCT’s picture

Status:Needs work» Needs review
StatusFileSize
new110.44 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,165 pass(es).
[ View ]
new563 bytes

oops. missed a conflict.
took out the use Drupal\breakpoint\Entity\Breakpoint as is done elsewhere in this patch.

mdrummond’s picture

Issue tags:+Needs reroll

Working with Lowell to hopefully get this to RTBC today. First step is to get a reroll done.

Lowell’s picture

Issue tags:-Needs reroll
StatusFileSize
new87.21 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

rerolled patch: menu_links removed as a dependency from toolbar

Status:Needs review» Needs work

The last submitted patch, 59: move-breakpoints-2271529-59.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 59: move-breakpoints-2271529-59.patch, failed testing.

mdrummond’s picture

Issue tags:+Needs reroll

Looks like some of the test removals didn't end up in the reroll.

Lowell’s picture

StatusFileSize
new109.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,152 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

rerolled patch...again: menu_links removed as a dependency from toolbar

mdrummond’s picture

Status:Needs work» Needs review

Go testbot go!

Status:Needs review» Needs work

The last submitted patch, 64: move-breakpoints2-2271529-64.patch, failed testing.

mdrummond’s picture

The ConfigTranslationListUITest is failing in #64, but wasn't in #57. No idea why. The patch removes the config entities for breakpoint and breakpoint group, and that's where the translation was applied to the labels for the breakpoint and breakpoint group. But that hasn't changed between 57 and 64, so not that that would cause a failure.

One thing I've noticed is that the config files for breakpoints in Stark and Seven haven't been removed, nor have the breakpoint settings been moved to the stark.info.yml and seven.info.yml files. Not sure if that could cause a problem here: I don't think so, but that is something that will need to be fixed.

We still need to address the variations found in #54 too.

The other item that needs to be fixed is that the breakpoint definitions were deleted from toolbar.info.yml. Those should be restored. We just needed to remove the dependency on menu_link for that file.

Might be worth making those changes to see if for some reason that fixes the three fails.

Jelle_S’s picture

Assigned:Unassigned» Jelle_S

I'll see if I can get the tests fixed and address all other points mentioned in #67.

Jelle_S’s picture

Assigned:Jelle_S» Unassigned
Status:Needs work» Needs review
StatusFileSize
new112.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,422 pass(es).
[ View ]

New patch, failing test from previous patch was green on my local machine.

mdrummond’s picture

Could we get an interdiff between #64 and #69 to see the differences?

Thanks for taking care of making these changes!

Jelle_S’s picture

StatusFileSize
new4.04 KB

Here's the interdiff ;-)

mdrummond’s picture

This is looking very good. We need some manual testing for this to make sure this works, ideally with some screen shots. Things to test:

1) Do breakpoints show up on picture mappings page?
2) If a new breakpoint is added after a theme is installed, does it show up on the picture mappings page?
3) If a breakpoint's values are changed after a theme is installed, does the change show up on the picture mappings page?
4) If a breakpoint is deleted after a theme is installed, does it no longer show up on the picture mappings page?
5) If breakpoint within a theme is removed, does it no longer show up on the picture mappings page?
6) If a theme is installed, does its breakpoints show up on the picture mappings page

It may be good to test this with a fresh D8 install, to make sure there is no lingering config data from before the patch.

After that, it should be RTBC.

One architectural question. Does a breakpoint group now refer to the set of breakpoints for a given theme or a given module?

A site might have multiple types of images: images that take up the full width of the content area or images that appear in sidebars, for example. Am I understanding this right that the theme info.yml file would now define all of the possible breakpoints for every image usage situation, and then you would create a subset of breakpoints appropriate for a particular image usage within the picture mappings on the Responsive Image admin page?

mdrummond’s picture

Status:Needs review» Reviewed & tested by the community

Reviewing this manually.

Starting with a fresh install.

Enabled Responsive Images module. Go to config page. Add responsive images mapping.

You can select a breakpoint group, which is all the breakpoints defined by a particular theme or module (only enabled themes or modules). You do not need to use every breakpoint within a given group. So each theme would want to define every potential breakpoint for all the different image usage scenarios, and then the mapping would be used to select just those breakpoints that are appropriate for a particular mapping. You can only use breakpoints within one particular group: you can't use some from one, and some from another. That makes sense.

I selected the Bartik breakpoint group, then added a mapping called Hero image. I mapped Thumbnail image style to Mobile breakpoint, Medium to Narrow breakpoint and Large to Wide breakpoint.

Then I went to add a new mapping to bartik.info.yml:

  giganto:
    name: giganto
    label: giganto
    mediaQuery: 'all and (min-width: 1200px)'
    weight: 3
    multipliers:
      1x: 1x

Cleared cache. Returned to mapping page. Edited Hero Image mapping. Giganto breakpoint is now available (yay!). Picked Large image style for Giganto.

Changed min-width of Giganto breakpoint to 1000px. Cleared cache. Changed value appears on Hero Image mappings page.

Removed Giganto breakpoint. Cleared cache. Re-loaded Hero Image mapping page. Giganto breakpoint no longer appears. Also does not appear when creating a new mapping.

Enabled Stark theme. When adding a responsive image mapping, Stark now appears as a breakpoint group. Added a mapping called Starky with the Stark breakpoint group.

Disabled Stark theme. When going to edit Starky, I need to select a new breakpoint group, since the Stark breakpoint group is no longer available.

The way this is working seems sensible to me. I don't see any flaws in the code that needs more fixes.

Setting this to RTBC. Entirely possible somebody will spot something I missed, but from my perspective this looks good. Getting this in will clean up a ton of breakpoint issues, then enable us to move forward to updating to the latest Picturefill, so that Drupal 8 can support the revised picture/srcset/sizes syntax that should be landing natively in browsers in September or October.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

No test coverage or usages of breakpoint_select_options() - needs a followup either to test or remove.

I don't think the hooks make sense. Especially given that breakpoint_rebuild() will fire delete and insert on all the existing breakpoints. We're still treating breakpoints and breakpoint groups as being configurable. I don't think the core implementation should at all. We're just caching the information from the .info.yml files. In fact I don't think we should have all this save/delete logic at all.

Why are we using state and not cache?

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new129.67 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,702 pass(es).
[ View ]

Here is a major rewrite of the patch and the breakpoint and responsive_image.

Breakpoints are stored in EXTENSION.breakpoints.yml in the root folder of the theme or module. Breakpoints are discovered and managed by the BreakpointManager which leverages the plugin system in the same way that, for example, contextual links does. Comparing HEAD, the patch #69 there is a considerable saving in complexity. The responsive image mapping has also been rewritten to sensibly stored mappings in a config safe array avoiding any possibility of dots in configuration keys. Methods have been added to the configuration entity to considerably reduced the complexity in using them.

Still @todo ensure that responsive mappings depend on the module or theme that provides the breakpoint group. This will ensure that stale configuration does not remain after uninstalling an extension and also that the user informed when uninstalling.

A contrib module can provide configurable breakpoints by implement an alter hook to change the definitions on the breakpoint plugin manager :)

The interdiff will be super confusing because this is a completely new approach so not providing.

dcrocks’s picture

Installed patch on a new clone of D8. Applied ok. After enabling responsive images tried to create a mapping with the bartik breakpoints. Return from 'save' operation said mapping was saved. Log messages said mapping was saved. Mapping exists in config table in database. But admin/configuration/responsive images page still says no mappings exist. Tried to add a field with a responsive image, but the drop down says no mappings. Can't test further at this point.

mdrummond’s picture

About to try testing this.

Quick note that stark.info.yml file still has breakpoints in it. Those probably need to be moved to a breakpoints.yml file, right?

mdrummond’s picture

Status:Needs review» Needs work
Issue tags:-Needs manual testing+Novice

I went through the same steps as I did in #73. I had to clear cache at every step of the way to make things work, and that's fine and makes sense.

Not sure what issues you ran into dcrocks, but maybe clearing cache could help. I ran into a few bugs the first time I tried this (there was a weird bug when I enabled responsive image module), and I think it was because I hadn't cleared cache. Ran a fresh install, cleared the heck out of the cache, and everything worked fine.

Only problem I spotted was the one I mentioned i #77, that Stark needs to have breakpoints in their own yml file rather than in info.yml. That's a novice task. Once we get that done, I am ok with RTBCing this. Would like to have attiks take a look, though, too.

Thanks for the overhaul, Alex. This seems like a more d8 way to do this, and I like that another module can come in to configure breakpoints. That should make it much easier to build a UI at some point, if we can agree on one. Cheers!

dcrocks’s picture

There are weird things going on after install(see #2307939: Update notifications are not enabled.. But I cleared cache 6 or 7 times with no change. The mapping file I created exists, but the fact it can't be seen by some forms may only be that the existing code is looking for a different naming syntax. And I have tried rebuilding multiple times.

dcrocks’s picture

In act, this might be a case of using 'mapping' instead of 'mappings'.

Jelle_S’s picture

Status:Needs work» Needs review
StatusFileSize
new136.42 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,748 pass(es), 23 fail(s), and 4 exception(s).
[ View ]
new8.92 KB

New patch:

  • Adds the dependency to the responsive image mappings
  • Moves the breakpoints from stark.info.yml to stark.breakpoints.yml
  • Clears cached breakpoints when modules or themes are enabled/disabled

@dcrocks I did not ran into any issues following your steps described in #76. The mapping showed up in the field formatter as expected. Maybe you should try a fresh D8 install?

Jelle_S’s picture

StatusFileSize
new136.71 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,736 pass(es).
[ View ]
new548 bytes

New patch, there was an issue where the cached breakpoint groups didn't get cleared.

The last submitted patch, 81: 2271529.81.patch, failed testing.

andypost’s picture

+++ b/core/modules/breakpoint/breakpoint.module
@@ -40,33 +37,29 @@ function breakpoint_help($route_name, RouteMatchInterface $route_match) {
+function breakpoint_modules_installed($installed_modules) {
...
+function breakpoint_modules_uninstalled($uninstalled_modules) {
...
+function breakpoint_themes_enabled($theme_list) {
...
+function breakpoint_themes_disabled($theme_list) {

+++ b/core/modules/breakpoint/breakpoint.services.yml
@@ -0,0 +1,6 @@
+      - { name: plugin_manager_cache_clear }

+++ b/core/modules/breakpoint/src/BreakpointManager.php
@@ -0,0 +1,194 @@
+  /**
+   * @var array
+   */
+  protected $breakpointsByGroup;
...
+    $this->setCacheBackend($cache_backend, 'breakpoints:' . $language_manager->getCurrentLanguage()->getId(), array('breakpoints' => TRUE));
...
+  public function getBreakpointsByGroup($group_name) {
...
+      if ($cache = $this->cacheBackend->get($this->cacheKey . ':' . $group_name)) {
+        $this->breakpointsByGroup[$group_name] = $cache->data;
...
+        $this->cacheBackend->set($this->cacheKey . ':' . $group_name, $breakpoints);
+        $this->breakpointsByGroup[$group_name] = $breakpoints;
...
+  public function getGroups() {
+    // Use a double colon so as to not clash with the cache for each group.
+    if ($cache = $this->cacheBackend->get($this->cacheKey . '::groups')) {
...
+    $this->cacheBackend->set($this->cacheKey . '::groups', $groups);
+    return $groups;
...
+  public function clearCachedDefinitions() {
+    foreach ($this->getGroups() as $group_name => $label) {
+      $this->cacheBackend->delete($this->cacheKey . ':' . $group_name);
+    }
+    $this->cacheBackend->delete($this->cacheKey . '::groups');
+    parent::clearCachedDefinitions();

It's not clear why this needed because all caches are cleared when extension is installed

Also seems manager forgets to clear it static $breakpointsByGroup (that needs docs)

andypost’s picture

Issue tags:-Novice

Also it looks strange that cacheKey uses language

Jelle_S’s picture

@andypost

When I enabled a theme, the cache was not cleared. I assumed the same would be true for modules.

alexpott’s picture

StatusFileSize
new28.69 KB
new140.72 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,624 pass(es), 78 fail(s), and 9 exception(s).
[ View ]

@Jelle_S nice work! Especially the theme disable cache clear!

Patch attached:

  • moves some of the provider logic to BreakpointManager from ResponsiveImageMapping::calculateDependencies - the config entity shouldn't know too much about breakpoints :)
  • moves stuff around in breakpoint to be consistent
  • adds to the unit test for the responsive image mapping configuration entity class
  • Plugin cache does not need to be cleared for modules this is handled automatically during module uninstall - themes are different (unfortunately)
  • Uses cache tags instead of a custom clearCachedDefinitions method

Working on an issue summary update.

@dcrocks steps to reproduce your problem would be awesome.

@andypost language is part of the cache key because some of the breakpoint definition is translatable - ie the label.

The diff is to #75 since I was working on this concurrently to @Jelle_S - I've merged in the improvements from #81 and #82

Status:Needs review» Needs work

The last submitted patch, 87: 2271529.87.patch, failed testing.

andypost’s picture

I found only a absence of some docs.
@alexpott I'm not sure that label is used somewhere... but it could use TranslationWrapper for, at least I found no services that uses this pattern for cache key

Also https://www.drupal.org/documentation/modules/breakpoint will need update

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandler.php
    @@ -664,4 +664,34 @@ protected function systemThemeList() {
    +  public function themeExists($theme) {

    +++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
    @@ -131,4 +131,39 @@ public function getBaseThemes(array $themes, $theme);
    +   * Determines whether a given theme is enabled.
    ...
    +  public function themeExists($theme);

    isThemeEnabled() looks better, exists is wrong in that context

  2. +++ b/core/modules/breakpoint/src/BreakpointManager.php
    @@ -0,0 +1,217 @@
    +  /**
    +   * @var array
    +   */
    +  protected $breakpointsByGroup;
    ...
    +    $this->breakpointsByGroup = NULL;

    var needs docs

  3. +++ b/core/modules/breakpoint/src/BreakpointManager.php
    @@ -0,0 +1,217 @@
    +  /**
    +   * @param string $group
    +   *
    +   * @return string
    +   *   The label
    +   */
    +  protected function getGroupLabel($group) {

    doc block needs clean-up

  4. +++ b/core/themes/stark/stark.breakpoints.yml
    @@ -0,0 +1,18 @@
    \ No newline at end of file

    :)

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new1.21 KB
new140.83 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,645 pass(es).
[ View ]

@andypost this is copied pretty much verbatim from contextual links manager

  1. This is a copy of the moduleHandler function - at some point I'd like ThemeHandler and ModuleHandler to be more consistent.
  2. Fixed
  3. Fixed
  4. Fixed

test fails in #87 look like bot issues.

dcrocks’s picture

StatusFileSize
new76.1 KB
new68.93 KB
new38.03 KB

Steps are pretty simple.
Make new clone of d8.0
Install
Enable Responsive_image module
Go to admin/config/media/responsive image mappings
Add a new mapping using the bartik breakpoints
Get success msg, but mapping not displayed. Clear caache multiple times with no affect.
I have provided images of the responsive image page after I saved the mapping, the message log showing it was saved, and a snip if the database showing the record for the mapping.

This actually looks like a problem I reported a while back(#2267563: Uncaught SQLite failure breaks responsive images).

attiks’s picture

#91 I tried it on a clean checkout and the patch from #90 and it works like it should.

dcrocks’s picture

Tried with patch in #90. Still a problem for me. Drupal seems to have a problem with being installed on VirtualDocumentRoot instead of DocumentRoot and has since alpha 10. But my problem shouldn't hold this up.

ps. I am using a different mac now and a different version of OS X, but all my d8 problems persist.

joelpittet’s picture

@dcrocks you may want to revisit your issue #2260969: 404 error on css/js aggregated files in site located in a VirtualDocumentRoot as most of the problems you are seeing stem from that eh?

dcrocks’s picture

Status:Needs review» Needs work

Patch no longer applies.

mdrummond’s picture

Issue tags:+Needs reroll

Blew away my install and still couldn't get patch to apply. Here's the error I get:

error: patch failed: core/modules/breakpoint/src/Tests/BreakpointGroupTestBase.php:1
error: core/modules/breakpoint/src/Tests/BreakpointGroupTestBase.php: patch does not apply
error: patch failed: core/modules/breakpoint/src/Tests/BreakpointTestBase.php:1
error: core/modules/breakpoint/src/Tests/BreakpointTestBase.php: patch does not apply
error: patch failed: core/modules/breakpoint/src/Tests/BreakpointThemeTest.php:1
error: core/modules/breakpoint/src/Tests/BreakpointThemeTest.php: patch does not apply
error: patch failed: core/modules/breakpoint/tests/src/BreakpointConfigEntityUnitTest.php:1
error: core/modules/breakpoint/tests/src/BreakpointConfigEntityUnitTest.php: patch does not apply
error: patch failed: core/modules/breakpoint/tests/src/BreakpointGroupConfigEntityUnitTest.php:1
error: core/modules/breakpoint/tests/src/BreakpointGroupConfigEntityUnitTest.php: patch does not apply
error: patch failed: core/modules/responsive_image/tests/src/ResponsiveImageMappingEntityTest.php:1
error: core/modules/responsive_image/tests/src/ResponsiveImageMappingEntityTest.php: patch does not apply

Something must have changed in these files. Reroll needed?

attiks’s picture

Status:Needs work» Needs review
StatusFileSize
new140.86 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,094 pass(es).
[ View ]

Rerolled

mdrummond’s picture

Assigned:Unassigned» mdrummond
Issue tags:-Needs reroll

Okay, got this to apply, so going through to do manual testing again.

mdrummond’s picture

I did the same set of manual tests I did for previous patches, and everything still works great. Looked through patch and changes files, and things are looking good. If patch comes back green (go testbot go!), then I think I could RTBC this.

mdrummond’s picture

Assigned:mdrummond» Unassigned

Status:Needs review» Needs work

The last submitted patch, 97: 2271529.97.patch, failed testing.

Status:Needs work» Needs review

marcingy queued 97: 2271529.97.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 97: 2271529.97.patch, failed testing.

Status:Needs work» Needs review

mdrummond queued 97: 2271529.97.patch for re-testing.

Status:Needs review» Needs work

The last submitted patch, 97: 2271529.97.patch, failed testing.

mdrummond’s picture

Errors look like testbot problems. Triggering retest.

Status:Needs work» Needs review

mdrummond queued 97: 2271529.97.patch for re-testing.

mdrummond’s picture

Status:Needs review» Reviewed & tested by the community

Okay, 97 is passed, I've done numerous manual tests of this, we've had a lot of eyes on this patch. A long time going into this. I'm marking this RTBC again, and hopefully we'll be set to get this in.

alexpott’s picture

Issue summary:View changes

Updated issue summary to reflect current solution.

When this is committed https://www.drupal.org/node/1813914 needs to be completely rewritten - it is hopelessly out of date with HEAD anyway :)

Jelle_S’s picture

Patch in #2260061-74: Responsive image module does not support sizes/picture polyfill 2.2 has already been created assuming this patch will be committed. This way we can move on a bit faster with the Picturefill issue.

webchick’s picture

Sigh. Had a much longer review and Safari's stupid "oh, you moved your mouse, OBVIOUSLY that means you meant to go back a page and obliterate your Dreditor review" behavior killed it. :( Mostly it was a bunch of nitpicks, tho.

The bigger points were:

- WOW! this patch is amazing!!! I can't get over this diffstat: 66 files changed, 1136 insertions, 2262 deletions.
- How is "label" going to get translated, if this is not running though CMI or other standard system? If it can't be translated, can we ditch it, in favor of the YAML key being the label, if that's ever exposed anywhere?
- (not for this patch/issue, pre-existing condition) The tests could really do with some comments explaining what they're testing and why.

Pausing on committing due to the label question.

attiks’s picture

We need the label, it is used in the picture mapping, so yes it has to be translatable.

Do you want it as part of this patch, or will a follow up works as well?

alexpott’s picture

Status:Reviewed & tested by the community» Needs review
Related issues:+#2322839: Unify YAML translation extraction, and allow extension by contrib.
StatusFileSize
new25.27 KB
new149.95 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2271529-translate.113.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Okay so this means we have to instantiate plugins - so we can get a translated label. This turns out to be great because now responsive image has completely no idea of the internal workings of the breakpoint module - it is just using methods on the breakpoint plugin and manager interfaces - yay!

Also

  • added more complete testing of the sorting of breakpoints by weight
  • tried to sort out translating groups - this needs review I think we I've done will work but this is complex because module and theme names should not be translated
  • injected the breakpoint manager where possible in the responsive image module

Also we need to make D8MI aware of this issue because it affects things like #2322839: Unify YAML translation extraction, and allow extension by contrib.

Gábor Hojtsy’s picture

Please open an issue in the potx module's queue and tag it with "Drupal 8 compatibility"

attiks’s picture

Jelle_S’s picture

Did a quick scan over the patch and found a small issue in BreakpointManager::getGroups():

+++ b/core/modules/breakpoint/src/BreakpointManager.php
@@ -0,0 +1,274 @@
+        if (!isset($groups[$plugin_definition['group']])) {
+          $groups[] = $plugin_definition['group'];
+        }

Should be (forgot the array key):

<?php
if (!isset($groups[$plugin_definition['group']]) {
 
$groups[$plugin_definition['group']] = $plugin_definition['group'];
}
?>

A more extensive review is still necessary though.

Status:Needs review» Needs work

The last submitted patch, 113: 2271529-translate.113.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 113: 2271529-translate.113.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 113: 2271529-translate.113.patch, failed testing.

mdrummond’s picture

Issue tags:+needs rerorll

Something changed in ThemeHandlerInterface.php, and this needs a reroll.

andypost’s picture

We need the label, it is used in the picture mapping, so yes it has to be translatable.

I found no usage after patch so open this question, only group name is used (or seems this is a extension name)

@attiks please point the loc of label usage! looks it's not needed at all

attiks’s picture

#123 It is used at http://cgit.drupalcode.org/drupal/tree/core/modules/responsive_image/src... which is altered by this patch to use the label. $label = $multiplier . ' ' . $breakpoint->getLabel() . ' [' . $breakpoint->getMediaQuery() . ']';

For clarity, the label is something like 'wide', which should be translatable. If we use the key (seven.wide) it will look strange

Example

seven.wide:
  label: wide
andypost’s picture

+++ b/core/modules/responsive_image/src/ResponsiveImageMappingForm.php
@@ -67,23 +92,23 @@ public function form(array $form, FormStateInterface $form_state) {
+        $label = $multiplier . ' ' . $breakpoint->getLabel() . ' [' . $breakpoint->getMediaQuery() . ']';
+        $form['keyed_mappings'][$breakpoint_id][$multiplier] = array(
...
+          '#title' => $label,

yep, it's really questionable how to translate this kind of strings

Wim Leers’s picture

Why not just label it Wide viewports?

andypost’s picture

StatusFileSize
new28.52 KB

How it looks:

mdrummond’s picture

This is user-provided texts. It doesn’t really matter what we create as the labels for the breakpoints in Bartik or other themes. Everybody creating their own sites will come up with their own sensible names for their breakpoints (or at least hopefully they will). That’s why these need to be translatable.

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new149.92 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,546 pass(es), 238 fail(s), and 115 exception(s).
[ View ]

Rerolledl

Status:Needs review» Needs work

The last submitted patch, 129: 2271529-translate.128.patch, failed testing.

The last submitted patch, 129: 2271529-translate.128.patch, failed testing.

Gábor Hojtsy’s picture

Is this ending up in *.info.yml files or the title / summary need updating? Looking at the patch it uses its own plugin YAML format?

mdrummond’s picture

Title:Move breakpoint settings to theme and module *.info.yml files» Move breakpoint settings to theme and module *.breakpoints.yml files at root level

Good catch

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new915 bytes
new150.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,104 pass(es).
[ View ]

Forgot to fix the toolbar module to use the new breakpoint methods.

Jelle_S’s picture

Did a review, most of them are comment nitpicks really.

  1. +++ b/core/lib/Drupal/Core/Extension/ThemeHandlerInterface.php
    @@ -139,4 +139,39 @@ public function getName($theme);
    +   * @param string $theme
    +   *   The name of the theme (without the .module extension).

    "without the .theme extension" ?

  2. +++ b/core/modules/breakpoint/src/BreakpointManager.php
    @@ -0,0 +1,274 @@
    +      foreach ($this->getDefinitions() as $plugin_definition) {
    +        if (!isset($groups[$plugin_definition['group']])) {
    +          $groups[] = $plugin_definition['group'];
    +        }
    +      }

    see #116

  3. +++ b/core/modules/breakpoint/src/BreakpointManager.php
    @@ -0,0 +1,274 @@
    +  /**
    +   * Gets the label for a breakpoint group.
    +   *
    +   * @param string $group
    +   *   The breakpoint group.
    +   *
    +   * @return string
    +   *   The label.
    +   */
    +  protected function getGroupLabel($group) {
    +    // Extension names are not translatable.
    +    if ($this->moduleHandler->moduleExists($group)) {
    +      $label = $this->moduleHandler->getName($group);
    +    }
    +    elseif ($this->themeHandler->themeExists($group)) {
    +      $label = $this->themeHandler->getName($group);
    +    }
    +    else {
    +      // Custom group label that should be translatable.
    +      $label = $this->t($group, array(), array('context' => 'breakpoint'));
    +    }
    +    return $label;
    +  }

    Is there a reason why this is not part of the BreakpointManagerInterface?

  4. +++ b/core/modules/breakpoint/tests/src/BreakpointTest.php
    @@ -0,0 +1,122 @@
    +/**
    + * @file
    + * Contains \Drupal\breakpoint\Tests\BreakpointTest
    + */

    Punctuation.

  5. +++ b/core/modules/breakpoint/tests/src/BreakpointTest.php
    @@ -0,0 +1,122 @@
    +  /**
    +   * Setups the local task default.
    +   */

    "Sets up the breakpoint defaults" ?

tim.plunkett’s picture

Is there a reason why this is not part of the BreakpointManagerInterface?

protected methods cannot be on interfaces, only public methods.

Jelle_S’s picture

StatusFileSize
new150.12 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,106 pass(es).
[ View ]
new1.86 KB

New patch with remarks form #135 fixed.

mdrummond’s picture

Status:Needs review» Reviewed & tested by the community

Did a quick run-through on simplytest.me. Nothing exploded. Looks to me like all remaining issues have been handled. Back to RTBC.

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Awesome, my feedback has been addressed. This is definitely going to be much easier for themers. Great work.

Committed and pushed to 8.x. Thanks!!!

  • webchick committed 75c4318 on 8.0.x
    Issue #2271529 by attiks, alexpott, Lowell, YesCT, Jelle_S | mdrummond:...
dcrocks’s picture

If anyone is still paying attention, it appears my problem is a SQLite issue. I broke down and installed mysql on my mac and installed D8 with that. Everything appears to work fine on this system.

dcrocks’s picture

Patch #2318191: [meta] Database tests fail on SQLite corrects the failing behavior I have been seeing. Still, not exactly sure why.

webchick’s picture

Awesome, thanks dcrocks!

Cottser’s picture

Issue tags:-needs rerorll

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.