[PLIP-Advisories] Re: [Plone] #9316: Unify folder implementations
plip-advisories at lists.plone.org
plip-advisories at lists.plone.org
Thu Oct 1 13:55:15 UTC 2009
#9316: Unify folder implementations
Reporter: smcmahon | Owner: witsch
Type: PLIP | Status: assigned
Priority: n/a | Milestone: 4.0
Component: Infrastructure | Resolution:
Replying to [comment:25 davisagli]:
> (In ) add FWT review, refs #9316
quoting parts of [browser:buildouts/plone-
coredev/branches/4.0/plips/plip9316-review-davisagli.txt at 29513 david's
review] to be able to provide comments inline:
> Notes and observations [[br]]
> * The original PLIP discussed the creation of a special view optimized
> viewing folders with many items. I presume this was dropped due to
> separate PLIP for revamping the collection UI? That's probably fine.
well, it wasn't really dropped because of that other PLIP, to be honest.
it was just that when martin & i started working on this back in
copenhagen, i never thought of a UI part, and i only read PLIP 191 again
after steve had already used it as the base for this one... :)
iow, it was never my intention to provide an update `folder_contents` view
for this, even though we have of course discussed the implications and
possible solutions for quite a bit. we've also created a slightly
adjusted version in one of our projects, but that's tailored towards the
partial ordering adapter.
in any case, i won't have time to come up and implement such a view as
part of this work. and i also think that it needs more discussion first.
sean upton's work (as part of this year's gsoc) seems promising, and i'd
very much like to see a PLIP for this for 4.1 or 4.2. it won't
necessarily me who's driving this, though... :)
> * The original PLIP also discussed some migration code for converting
> the internal data structures of existing folders. Does this code
> where where it can be reviewed?
not yet, it still lives inside a migration package of the project it was
developed for. i will merge it into `plone.app.folder` next thing,
> It's crucial that we get this right, of
> course, and that it can be made to work for custom subclasses of
> as well. Assuming this concern is addressed, it looks like custom
> will simply pick up the new mixin and "just work" as long as they
> overriding the old ordering implementation.
right. the migration has proven to work for our project: some 15+k
folders were migrated a while ago and are now used in production by
several thousand users every (week)day. so far we haven't seen any
problems with neither the migration nor the new folder implementation.
however, i should mention that this project is using the partial ordering
adapter instead of the default one.
> * The extension profile in plone.app.folder appears to merely duplicate
> change that was made in CMFPlone's Folder.xml.
right, and the change to the default `Folder.xml` was bad anyway, see
r30089. the same commit also keeps the extra profile from being loaded,
fixing this issue. you are right, though, the profile is indeed still
needed for plone 3.x.
> * bin/alltests shows erroring tests in a bunch of packages, mostly with
a traceback and error similar to the following:
this was also fixed by r30089 — the problem was that the factory (in
`plone.app.folder`) hadn't been properly initialized yet by the time PTC
was trying to create a folder during site setup. as `ATFolder` itself is
now patched (by this PLIP, as opposed to the stand-alone
`plone.app.folder` for 3.x), the factory can simply remain to be the old
> ValueError: Product factory for Folder was invalid
on a side node, i only realized last night that i left the PLIP in a
completely unfinished state the night i was trying to wrap it up for the
first round of reviews. i found a yet uncommitted change and also
remembered having seen that error before. however, i also remember
constantly dozing off over the keyboard that night — no real surprise at
3am, i guess...
at some point i think i gave up on the above mentioned test setup problem,
deciding i would look into it in the morning. however, the funny thing to
me is that apparently i was so tired that i didn't even remember something
still wasn't quite working the next day. judging from my time tracking i
must have thought everything was fine and never looked back... so again,
sorry about the mess. :)
> There are also a lot of test issues in Archetypes.
right, there are indeed a number of test issues left. i've just updated
the branches and checked a couple of packages:
* `Product.Archetypes` currently has 5 failures, 9 errors (these are
very likely all because the sample type `SimpleBTreeFolder` can for some
reason not be adapted to `IOrdering` anymore)
* `Product.ATContenTypes` has 1 failure in `traversal.txt`
* `Product.CMFPlone` has 0 failures
* `plone.folder` has 0 failures (needs to be run using `bin/instance
test -s plone.folder` for some reason)
* `plone.app.folder` (which also needs to be run using `bin/instance
test ...`) has 0 failures, but a problem with importing
`test_integration.py` (i suspect a circular import or something)
i will investigate these asap, but none of them should be show-stoppers.
the same tests all pass on plone 3.x, and i'm pretty sure the failures are
only triggered because of the integrational changes in `Archetypes` and
> I assume that these are
> mostly due to the switch to Plone 4 when this package was developed for
> 3, but come on, these really need to be fixed for this PLIP to be
you're right of course. sorry about turning this in without properly
finishing up first. but like i said, i thought i had... %)
> * I did not have time to verify the benchmarks included in the PLIP
> but don't have reason to doubt them.
actually, it's pretty easy to run them — see the comments in
[browser:plone.app.folder/trunk/src/plone/app/folder/tests/benchmarks.py at 30097
benchmark.py]. the [browser:plone.folder/jmeter at 30097 jmeter tests] will
be trickier to set up, of course.
> * Any sense of how this affects pickle size and transaction size for
> on different sizes of folders?
no, i haven't checked yet. would you have any suggestions on what to test
here in particular?
Ticket URL: <http://dev.plone.org/plone/ticket/9316#comment:29>
Plone Content Management System
More information about the PLIP-Advisories