Plip 3395 plone base #3396

jensens · 2022-01-09T19:23:04Z

mister-roboto · 2022-01-09T19:23:08Z

@jensens 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!

mauritsvanrees · 2022-03-25T21:48:44Z

I get strange tracebacks when I run a test. The test is chosen randomly, I just happened to have it in my bash history.

$ bin/buildout -c plips/plip-3395-plone.base-part-1.cfg
$ bin/test -s Products.CMFPlone -m testMigrationTool
...
Traceback (most recent call last):
  File "/Users/maurits/shared-eggs/cp39/plone.testing-8.0.3-py3.9.egg/plone/testing/zope.py", line 273, in zopeApp
    app.REQUEST.close()
  File "/Users/maurits/community/plone-coredev/6.0/src/Zope/src/ZPublisher/BaseRequest.py", line 215, in close
    notify(EndRequestEvent(None, self))
  File "/Users/maurits/shared-eggs/cp39/zope.event-4.5.0-py3.9.egg/zope/event/__init__.py", line 32, in notify
    subscriber(event)
  File "/Users/maurits/shared-eggs/cp39/zope.component-5.0.1-py3.9.egg/zope/component/event.py", line 27, in dispatch
    component_subscribers(event, None)
  File "/Users/maurits/shared-eggs/cp39/zope.component-5.0.1-py3.9.egg/zope/component/_api.py", line 134, in subscribers
    return sitemanager.subscribers(objects, interface)
  File "/Users/maurits/shared-eggs/cp39/zope.interface-5.4.0-py3.9-macosx-10.15-x86_64.egg/zope/interface/registry.py", line 448, in subscribers
    return self.adapters.subscribers(objects, provided)
ZODB.POSException.POSKeyError: 'RequestContainer' object has no attribute 'adapters'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/maurits/shared-eggs/cp39/zope.testrunner-5.4.0-py3.9.egg/zope/testrunner/runner.py", line 467, in run_layer
    setup_layer(options, layer, setup_layers)
  File "/Users/maurits/shared-eggs/cp39/zope.testrunner-5.4.0-py3.9.egg/zope/testrunner/runner.py", line 837, in setup_layer
    setup_layer(options, base, setup_layers)
  File "/Users/maurits/shared-eggs/cp39/zope.testrunner-5.4.0-py3.9.egg/zope/testrunner/runner.py", line 837, in setup_layer
    setup_layer(options, base, setup_layers)
  File "/Users/maurits/shared-eggs/cp39/zope.testrunner-5.4.0-py3.9.egg/zope/testrunner/runner.py", line 842, in setup_layer
    layer.setUp()
  File "/Users/maurits/shared-eggs/cp39/plone.app.testing-7.0.0a1-py3.9.egg/plone/app/testing/layers.py", line 105, in setUp
    self.setUpDefaultContent(app)
  File "/Users/maurits/.pyenv/versions/3.9.9/lib/python3.9/contextlib.py", line 126, in __exit__
    next(self.gen)
  File "/Users/maurits/shared-eggs/cp39/plone.testing-8.0.3-py3.9.egg/plone/testing/zope.py", line 279, in zopeApp
    raise inner_exception
  File "/Users/maurits/shared-eggs/cp39/plone.testing-8.0.3-py3.9.egg/plone/testing/zope.py", line 268, in zopeApp
    transaction.commit()
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_manager.py", line 257, in commit
    return self.manager.commit()
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_manager.py", line 134, in commit
    return self.get().commit()
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_transaction.py", line 282, in commit
    reraise(t, v, tb)
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_compat.py", line 49, in reraise
    raise value
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_transaction.py", line 273, in commit
    self._commitResources()
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_transaction.py", line 456, in _commitResources
    reraise(t, v, tb)
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_compat.py", line 49, in reraise
    raise value
  File "/Users/maurits/shared-eggs/cp39/transaction-3.0.1-py3.9.egg/transaction/_transaction.py", line 430, in _commitResources
    rm.commit(self)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/Connection.py", line 492, in commit
    self._commit_savepoint(transaction)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/Connection.py", line 1046, in _commit_savepoint
    obj._p_estimated_size = len(data)
  File "/Users/maurits/community/plone-coredev/6.0/src/Products.CMFPlone/Products/CMFPlone/Portal.py", line 69, in __setattr__
    if self._tree is not None and name in self:
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/Connection.py", line 791, in setstate
    p, serial = self._storage.load(oid)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/mvccadapter.py", line 153, in load
    r = self._storage.loadBefore(oid, self._start)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/DemoStorage.py", line 225, in loadBefore
    return self.base.loadBefore(oid, tid)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/DemoStorage.py", line 225, in loadBefore
    return self.base.loadBefore(oid, tid)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/utils.py", line 288, in __call__
    return func(*args, **kw)
  File "/Users/maurits/shared-eggs/cp39/ZODB-5.6.0-py3.9.egg/ZODB/MappingStorage.py", line 168, in loadBefore
    raise ZODB.POSException.POSKeyError(oid)
ZODB.POSException.POSKeyError: 0x2d60390859e2c763

I think I saw a similar traceback recently, but can't remember what caused it. Well, might have been around the zope.component upgrade. Ah, indeed, see some comments here: plone/plone.app.users#107
But when I switch back to zope.component 4.6.2 I get the same error. So may be something else entirely.

mauritsvanrees · 2022-03-25T21:53:59Z

Slightly easier: bin/test -u fails with the same traceback, during test layer setup. End result is:

  Ran 2040 tests with 6 failures, 79 errors, 2 skipped in 16.880 seconds.

mauritsvanrees · 2022-03-30T08:28:48Z

I confirm the tests pass locally (at least the ones that failed earlier for me). I have restarted the PLIP jobs.

jensens · 2022-03-30T12:31:20Z

FTR: link to plip-jobs/3.9 https://jenkins.plone.org/view/PLIPs/job/plip-3395-plone.base-part-1-3.9/

jensens · 2022-03-30T12:32:30Z

And PLIP jobs are green!

mauritsvanrees · 2022-03-30T12:51:24Z

For me it would be good to merge.

@plone/framework-team have a look at the implementation please.

ale-rt · 2022-04-05T16:21:55Z

I had a look to the plone.base package but I am struggling to find the time for an in depth review.

It seems well crafted but I have some doubts.

The defaultpage module pulls in quite a lot of dependencies that otherwise could be dropped and the content is not really something that survived well the time.
Does it really make sense to move it to a new package?

I also noted this deprecation in the batch module with a long explanation.

That also feels weird, either we revert the deprecation or remove the code.

That said I already asked if somebody of the @plone/framework-team has some time to review this one.
If in the next week:

the tests are green again
you think the things I mentioned above are not blockers or something which is worth investing time fixing
nobody else from the @plone/framework-team jumps in to (at least) propose review this one

I would just say that you should feel free to push the merge button.

ale-rt · 2022-04-05T16:24:59Z

One other thing... I think we lost some tests.
See https://github.com/plone/plone.base/blob/6e26a377f7153ea6631a6ad58b7a9008fd3d6e53/src/plone/base/utils.py#L69

It mentions safe_unicode which is not something we have in that package.
Probably it is way easier to move those doctests in some unitest.

jensens · 2022-04-05T21:45:32Z

The defaultpage module pulls in quite a lot of dependencies that otherwise could be dropped and the content is not really something that survived well the time.

Indeed. But this might get refactored later. OTOH defaultpage is used by a bunch of plone.app.* packages, so it has to be moved to somewhere. Probably to Products.CMFDynamicViewFTI? But we can move it further later, right? I am happy having it that far down for now.

I also noted this deprecation in the batch module with a long explanation.

That also feels weird, either we revert the deprecation or remove the code.

This is true, but IMO out of scope here. We can tackle this on a different PR later.

jensens · 2022-04-05T21:54:12Z

One other thing... I think we lost some tests. See https://github.com/plone/plone.base/blob/6e26a377f7153ea6631a6ad58b7a9008fd3d6e53/src/plone/base/utils.py#L69

It mentions safe_unicode which is not something we have in that package. Probably it is way easier to move those doctests in some unitest.

It seems in a former copy paste session the docstring was copied, but never reviewed. The test does not make any sense for safe_text and been probably never run. This would need indeed some love, but also IMO out of scope here.

mauritsvanrees · 2022-04-05T22:32:52Z

@jenkins-plone-org please run jobs

mauritsvanrees · 2022-04-05T22:36:20Z

Oops, starting Jenkins via a comment here is not very useful. I stopped the jobs and started the proper PLIP jobs instead.

jensens · 2022-04-06T10:44:35Z

All green. IMO ready for merge?

petschki

LGTM

jensens added 13 commits January 9, 2022 11:07

first short for PLIP plone.base

2cc2515

fix imports

5852da7

fix safe_unicode import to safe_text

07c75f9

fix safeToInt import and isort

37c1b7e

fix get_top_request

79d0d35

deprecate log_deprecated

99156b7

fix safe_text import

411ba9b

fix get_top_request import

e86f71a

bring back log*

192d5a6

move tests from cmfplone to plone.base

ecdc32a

doctest moved to plone.base

714221b

safe_nativestring -> safe_text

bc60af2

doc chnages, at least some

7f15d29

jensens added 04 type: enhancement 12 prio: high 22 status: in-progress labels Jan 9, 2022

jensens mentioned this pull request Jan 9, 2022

PLIP 3395 plone.base: Untangles existing circular dependencies (imports/zcml) in Plone. #3395

Closed

48 tasks

Better chnagelog.

d926543

jensens added 2 commits March 25, 2022 17:16

merged master

8c379b7

remove glitch after merge

06dc3b8

Merge branch 'master' into PLIP-plone-base

26c87d8

pbauer mentioned this pull request Mar 29, 2022

Savepoint required in PloneFixture for tests to pass #3467

Closed

pbauer and others added 2 commits March 29, 2022 21:45

Merge branch 'master' into PLIP-plone-base

dd7ed4d

Merge branch 'master' into PLIP-plone-base

6ce4a94

jensens added 24 status: ready and removed 22 status: in-progress labels Mar 30, 2022

Merge branch 'master' into PLIP-plone-base

bad9ad0

Merge branch 'master' into PLIP-plone-base

d5cf031

petschki approved these changes Apr 6, 2022

View reviewed changes

jensens requested a review from petschki April 6, 2022 13:35

jensens merged commit a81808b into master Apr 6, 2022

jensens deleted the PLIP-plone-base branch April 6, 2022 13:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plip 3395 plone base #3396

Plip 3395 plone base #3396

jensens commented Jan 9, 2022

mister-roboto commented Jan 9, 2022

mauritsvanrees commented Mar 25, 2022

mauritsvanrees commented Mar 25, 2022

mauritsvanrees commented Mar 30, 2022

jensens commented Mar 30, 2022

jensens commented Mar 30, 2022

mauritsvanrees commented Mar 30, 2022

ale-rt commented Apr 5, 2022

ale-rt commented Apr 5, 2022

jensens commented Apr 5, 2022

jensens commented Apr 5, 2022

mauritsvanrees commented Apr 5, 2022

mauritsvanrees commented Apr 5, 2022

jensens commented Apr 6, 2022

petschki left a comment

Plip 3395 plone base #3396

Plip 3395 plone base #3396

Conversation

jensens commented Jan 9, 2022

mister-roboto commented Jan 9, 2022

mauritsvanrees commented Mar 25, 2022

mauritsvanrees commented Mar 25, 2022

mauritsvanrees commented Mar 30, 2022

jensens commented Mar 30, 2022

jensens commented Mar 30, 2022

mauritsvanrees commented Mar 30, 2022

ale-rt commented Apr 5, 2022

ale-rt commented Apr 5, 2022

jensens commented Apr 5, 2022

jensens commented Apr 5, 2022

mauritsvanrees commented Apr 5, 2022

mauritsvanrees commented Apr 5, 2022

jensens commented Apr 6, 2022

petschki left a comment

Choose a reason for hiding this comment