Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add upgrade profile and step to update the bundle timestamp. #248

Closed
wants to merge 1 commit into from

Conversation

mauritsvanrees
Copy link
Member

This is a follow-up on #247 (comment)
That PR updated the actions, with an upgrade step, but also changed css, which was missing an upgrade step. That is what I do here. This is an upgrade profile, so we have one upgrade step that applies the actions, updates the bundle timestamp, and combines the bundles.

I first tried it smaller, with only a tiny upgrade step, but that was not enough.
See branch maurits/upgrade-step-bundle-1, commit e15cd7d and the commented-out ideas in that commit.

cc @thet because I wonder if he knows something simpler than either this PR or the ideas in the commit I reference.

I could imagine a new function in CMFPlone that takes a bundle prefix and a timestamp, and sets the last compilation date on this bundle, and calls combine-bundles, taking care that the ContentType of the response is not changed to json. So roughly the update_last_compilation function I have in commit e15cd7d, plus the combine-bundles from the comments there.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.093% when pulling 4371932 on maurits/upgrade-step-bundle-2 into 3d3efd0 on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.093% when pulling 4371932 on maurits/upgrade-step-bundle-2 into 3d3efd0 on master.

@fredvd
Copy link
Member

fredvd commented Sep 8, 2020

@mauritsvanrees The thing we could add here as well or add officially to core Plone is what we have done in some projects: Create a simple function which imports and runs combine_bundles:

from Products.CMFPlone.resources.browser.combine import combine_bundles

def compile_bundles(context):
    """Call combine_bundles directly until we can use combine-bundles in
       an upgrade-depends in Plone 5.1.4 or higher"""

    combine_bundles(context)

And run it in an upgrade step with a handler:

      title="compile bundles"
      description=""
      handler=".setuphandlers.compile_bundles"/>

We 'know' the css has been changed, it's already 'compiled', but only needs to be added to a refreshed metabundle.
It's a different case if you need to compile less to css as well, then you need the full compilation step a ++plone++ namespace etc.

Why again did we add it to the local policy package its setuphandlers, probably because

  handler="Products.CMFPlone.resources.browser.combine.compile_bundles"

doesn't work? Or does it?

EDIT: Ahem, it's getting too late. I'm copying above the comment with the clarification without reading it. It does after Plone 5.1.4.

@mauritsvanrees
Copy link
Member Author

handler="Products.CMFPlone.resources.browser.combine.compile_bundles"

should be:

handler="Products.CMFPlone.resources.browser.combine.combine_bundles"

And you cannot use it well because it gets javascript resources, which has the side effect that it changes the response Content-Type to application/javascript. This is what the combine-bundles import step guards against.

Ah, but I see this was fixed in the base combine_bundles four year ago.

It gets a bit confusing, because you have this basic combine_bundles function that combines the bundles, and you have the combine-bundles import step which is handled by Products.CMFPlone.resources.exportimport.bundles.combine.

The import step only does something if there is a registry.xml (or registry/*.xml) with an IBundleRegistry setting in the profile, and then it calls the basic combine_bundle function. And then it still undoes the possible update Content-Type update, but that part is not needed anymore, as I saw just now.

I was not sure if you could use the combine-bundles import step as upgrade step, because it may not find the registry.xml file. But I see this is not a problem: it finds it just fine.

Summary: my approach in this PR works, but it can be much simpler than I feared.
Let me try again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants