The Stylizer may use the same CSS ID for multiple panes/panels with the same palette settnigs:

 function ctools_stylizer_get_css_id($plugin, $settings) {
  return 'ctools-stylizer:' . $settings['name'] . ':' . md5(serialize($settings['palette']));
 }

As a result, it may lead to issues saving Stylizer settings.
This may also cause https://www.drupal.org/node/2759869 .

Comments

abramm created an issue. See original summary.

abramm’s picture

Status: Active » Needs review
StatusFileSize
new1.46 KB

Patch attached.

id.tornado’s picture

Status: Needs review » Reviewed & tested by the community

it works for me

geek-merlin’s picture

Issue tags: +low-regression-risk

Only CSS IDs changed.

joelpittet’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -low-regression-risk

How does this "work for you"? And more importantly how does this manifest as a bug?

It's also changing a path and a name as well as the CSS id.

geek-merlin’s picture

Status: Needs review » Reviewed & tested by the community

> how does this manifest as a bug?

IS:
> The Stylizer may use the same CSS ID for multiple panes/panels with the same palette settnigs:

This is evident from the cited code.
Duplicate CSS IDs are a bug in itself (invalid HTML, maybe SEO).
Code change is trivial and makes a simple change to hashing all settings.

geek-merlin’s picture

Status: Reviewed & tested by the community » Needs review

Ups

joelpittet’s picture

Ok so ctools_css_clear($id) will fail to remove the old CSS. May need to clear out ctools-stylizer: files in an update hook?

geek-merlin’s picture

No prob, will be done on next CC via ctools_css_flush_caches().

joelpittet’s picture

That's a good point it clears public://ctools/css although doesn't clear public://ctools/style.
ctools_stylizer_cleanup_style() Tries but uses the generated name's specific path to delete generated images.

I appreciate you trying to ease my mind on this, I don't want to cause a unnecessary regression.

geek-merlin’s picture

It's good you're critical, maybe i've been not thorough enough.
Now i see that stylyzer not only builds CSS, but some preview image bundled in a dir. ctools_stylizer_get_settings_name() tells us its name is the stylyzer name (so no problem here) OR for temp styles "_temporary-[MD5]".

The biggest problem from that might be some rare leftover files with no regression risk. Preview icons will be rebuilt on style resave.
Would be fine for me, but i won't prevent perfectionists from implementing a hook_update ;-).

abramm’s picture

Sorry, I wasn't probably clear enough when I created the issue.

The problem with IDs is that the same style may apply to different content panes.
I.e having panes A and B both using Stylizer with the same palette settings but different font settings, the same font setting will apply to both.

As for an update hook, how about fetching all Stylizer files from ctools_css_cache? E.g.:

SELECT * FROM {ctools_css_cache} WHERE cid LIKE 'ctools-stylizer:%';

We can then walk through all files in the update hook batch callback and call ctools_css_clear(). That will remove all generated Stylizer CSS files.

japerry’s picture

Status: Needs review » Closed (outdated)

Drupal 7 is no longer supported, closing.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.