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

Resolve circular dependency between Products.CMFPlone and plone.i18n. #2769

Merged
merged 3 commits into from Feb 22, 2019

Conversation

4 participants
@sallner
Copy link
Contributor

sallner commented Feb 20, 2019

In plone/plone.i18n#30 the interface was integrated in plone.i18n. This PR uses it in CMFPlone.

Fixes #2049

@sallner sallner requested a review from jensens Feb 20, 2019

@mister-roboto

This comment was marked as outdated.

Copy link

mister-roboto commented Feb 20, 2019

@sallner thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented Feb 20, 2019

@jenkins-plone-org please run jobs

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented Feb 20, 2019

I am unsure with this line

<records interface="Products.CMFPlone.interfaces.ILanguageSchema"
Should this be integrated in plone.i18n or should the dependency point to plone.i18n as there are just dependencies to Products.CMFPlone declared in this file, I don't know how to proceed?

@sallner sallner requested a review from gforcada Feb 20, 2019

@gforcada

This comment has been minimized.

Copy link
Contributor

gforcada commented Feb 20, 2019

IMHO I would keep it here and update the reference.

This way, plone.i18n does not need a registry.xml.

We might need an upgrade step to copy over the settings I guess, but someone with more upgrade-foo should comment @mauritsvanrees or @pbauer, could you? 😃

@jensens
Copy link
Member

jensens left a comment

Overall LGTM! Except the one missing the one deprecation import.

The only open task is now an upgrade step in plone.app.upgrade transforming the current settings in the registry to the new interface w/o loosing data. I would need also to dig deeper on that to see how to do that. So I hope someone else knows!

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented Feb 21, 2019

@jenkins-plone-org please run jobs

@gforcada gforcada closed this Feb 21, 2019

@gforcada gforcada reopened this Feb 21, 2019

@mister-roboto

This comment has been minimized.

Copy link

mister-roboto commented Feb 21, 2019

@sallner thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

@gforcada

This comment has been minimized.

Copy link
Contributor

gforcada commented Feb 21, 2019

Sorry I closed it by accident 😅

@sallner

This comment has been minimized.

Copy link
Contributor Author

sallner commented Feb 21, 2019

@jenkins-plone-org please run jobs

and there is goes again 🌀

@jensens jensens merged commit c90f5c2 into master Feb 22, 2019

5 checks passed

Changelog verifier Entry found
Details
Plone Contributors Agreement verifier All users have signed it
Details
Plone Jenkins CI - pull-request-5.2 Job finished with success status
Details
Plone Jenkins CI - pull-request-5.2-3.6 Job finished with success status
Details
Plone Jenkins CI - pull-request-5.2-3.7 Job finished with success status
Details

@jensens jensens deleted the 2049-circular-dep-plone.i18n branch Feb 22, 2019

@jensens

This comment has been minimized.

Copy link
Member

jensens commented Feb 22, 2019

@sallner thanks so far, now there is "only" the upgrade step missing migrating existing registry records to the new location. Would you give this a try? I would start by asking this as a question at https://community.plone.org

@gforcada

This comment has been minimized.

Copy link
Contributor

gforcada commented Feb 22, 2019

@jensens I hold back on merging this one until we had the upgrade steps, merging the plone.i18n was fine as even if it gets released, no one is pointing to that interface there, so no harm is done.

Here though, we need the plone.i18n released at the same time, but specially the upgrade steps, if not, anyone trying to upgrade from 5.2alphas or betas will get a non-working language control panel.

And most of it all I'm saying it because in a week, after the plone tagung, I know that a new release will be asked to the release.

So getting those upgrade steps will definitely be a blocker 😉

Now, you have something to work on the sprints 😜

@jensens

This comment has been minimized.

Copy link
Member

jensens commented Feb 22, 2019

I will look at this before release if nobody else does. Should not be that difficult to do, need to dig a bit into registry internals.

@gforcada

This comment has been minimized.

Copy link
Contributor

gforcada commented Feb 22, 2019

I don't think you need to go to the registry internals, just plain old copy && paste && delete:
pseudo code ahead:

foo1 = api.get_registry_value(my_field)
api.set_registry_value(my_new_field=foo1)
foo2 = api.get_registry_value(my_field)
api.set_registry_value(my_new_field=foo2)
foo3 = api.get_registry_value(my_field)
api.set_registry_value(my_new_field=foo3)

the interface is still importable from CMFPlone, so it should not break getting the value...

@esteele esteele added this to Critical in Plone 5.2 final Mar 14, 2019

@jensens jensens moved this from Critical to Done in Plone 5.2 final Mar 26, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.