Mailing List Archive: Zope: CMF

IndexableObjectWrapper

1 2 3

View All

Index | Next | Previous | View Threaded

miles at jamkit

Mar 18, 2009, 4:21 AM

Post #51 of 60 (1150 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Hi,

>>> - The CatalogTool tests set up the adapter at the moment, as a lot of
>>> the catalog tests require the adapter to work properly. This is done in
>>> the _makeContent method as it applied to most tests that used the dummy
>>> content. However, I think it belongs somewhere else, but I wasn't sure
>>> whether that place was a layer, a setup method or somewhere else. Any
>>> suggestions?
>> I agree it belongs somewhere else. Maybe a registerWrapper method. But
>> can't we make the adapter lookup in catalog_object optional and wouldn't
>> that make test setups simpler?
>
> Agreed. I had expected that the catalog would do a queryAdapter, and
> default to the existing wrapper class if not found.

Makes sense for BBB - it's possible that someone might be inheriting
from the Catalog but not loading the adapter registrations, in which
case their code would just break.

Can I suggest the following logic:

1. if the object already implements the IIndexableObject marker
interface, no wrapping is required;

2. otherwise, adapt to IIndexableObject to do the wrapping;

3. if no adapter is registered, fall back to the existing
IndexableObjectWrapper class for BBB.

In the case of 3, I would like to issue some deprecation warning or log
message to alert that a registration is required in future. As 2.2.0 is
not yet released, is it possible/desirable to make it a deprecation?

Miles

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

tseaver at palladion

Mar 18, 2009, 4:27 AM

Post #52 of 60 (1149 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Miles wrote:
>
> Hi,
>
>>>> - The CatalogTool tests set up the adapter at the moment, as a lot of
>>>> the catalog tests require the adapter to work properly. This is done in
>>>> the _makeContent method as it applied to most tests that used the dummy
>>>> content. However, I think it belongs somewhere else, but I wasn't sure
>>>> whether that place was a layer, a setup method or somewhere else. Any
>>>> suggestions?
>>> I agree it belongs somewhere else. Maybe a registerWrapper method. But
>>> can't we make the adapter lookup in catalog_object optional and wouldn't
>>> that make test setups simpler?
>> Agreed. I had expected that the catalog would do a queryAdapter, and
>> default to the existing wrapper class if not found.
>
> Makes sense for BBB - it's possible that someone might be inheriting
> from the Catalog but not loading the adapter registrations, in which
> case their code would just break.
>
> Can I suggest the following logic:
>
> 1. if the object already implements the IIndexableObject marker
> interface, no wrapping is required;
>
> 2. otherwise, adapt to IIndexableObject to do the wrapping;
>
> 3. if no adapter is registered, fall back to the existing
> IndexableObjectWrapper class for BBB.

That sounds like what I had in mind, but not for BBB. I think of the
adapter scheme as a way to choose a non-default wrapper, rather than a
quasi-mandatory replacement for it.

> In the case of 3, I would like to issue some deprecation warning or log
> message to alert that a registration is required in future. As 2.2.0 is
> not yet released, is it possible/desirable to make it a deprecation?

I wouldn't add a warning for it, because I wouldn't rip out the fallback.

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 tseaver [at] palladion
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJwNqF+gerLs4ltQ4RArehAJ9PSt1zNnJFdKJV/dPx096ZP8y8/wCgmYSF
CAD9dshczPrdWP4uf6cnpsQ=
=/BUg
-----END PGP SIGNATURE-----

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

tseaver at palladion

Mar 18, 2009, 5:12 AM

Post #53 of 60 (1155 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Miles wrote:
>
> Hi
>
> <snip>
>
> >> Can I suggest the following logic:
> >>
> >> 1. if the object already implements the IIndexableObject marker
> >> interface, no wrapping is required;
> >>
> >> 2. otherwise, adapt to IIndexableObject to do the wrapping;
> >>
> >> 3. if no adapter is registered, fall back to the existing
> >> IndexableObjectWrapper class for BBB.
> >
> > That sounds like what I had in mind, but not for BBB. I think of the
> > adapter scheme as a way to choose a non-default wrapper, rather than a
> > quasi-mandatory replacement for it.
>
> Ok, well this logic is checked in now on the branch, and tests adjusted
> accordingly. Without any warnings.

Thanks, looks good.

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 tseaver [at] palladion
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJwOUj+gerLs4ltQ4RAolpAKC0o4oox/IYKe4curmEgKFiLnRjqQCg2kd2
Soip97b3h35ItbJhdjKjxNA=
=+baG
-----END PGP SIGNATURE-----

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

y.2009 at wcm-solutions

Mar 18, 2009, 5:48 AM

Post #54 of 60 (1160 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Hi!

Tres Seaver wrote:
> Miles wrote:
>> >> Can I suggest the following logic:
>> >>
>> >> 1. if the object already implements the IIndexableObject marker
>> >> interface, no wrapping is required;

If we don't support 3., we can make 'no wrapping' the default.

>> >> 2. otherwise, adapt to IIndexableObject to do the wrapping;
>> >>
>> >> 3. if no adapter is registered, fall back to the existing
>> >> IndexableObjectWrapper class for BBB.
>> >
>> > That sounds like what I had in mind, but not for BBB. I think of the
>> > adapter scheme as a way to choose a non-default wrapper, rather than a
>> > quasi-mandatory replacement for it.

What's the win of providing a default that way? IndexableObjectWrapper
contains policy decisions, Plone e.g. doesn't use it. The current code
on the branch registers an adapter for IContentish, so CMFDefault will
never use that hardcoded default.

The change is in a new feature release. People can't expect full BBB if
they use customized registrations or catalog content that doesn't
implement IContentish.

>> Ok, well this logic is checked in now on the branch, and tests adjusted
>> accordingly. Without any warnings.
>
> Thanks, looks good.

Looks unnecessarily complex to me. But I'm afraid I'm outvoted.

Cheers,

Yuppie

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

charlie at begeistert

Mar 18, 2009, 6:14 AM

Post #55 of 60 (1151 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Am 18.03.2009 um 13:48 schrieb yuppie:

> Looks unnecessarily complex to me. But I'm afraid I'm outvoted.

I wouldn't say that. From what I've understood of the discussion I
tend to favour your position. Unfortunately I don't think I've
understood everything well enough to make a really informed decision!
I do, however, like the idea of the default adapter.

Charlie
--
Charlie Clark
Helmholtzstr. 20
D�sseldorf
D- 40215
Tel: +49-211-938-5360
GSM: +49-178-782-6226

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

miles at jamkit

Mar 18, 2009, 6:18 AM

Post #56 of 60 (1152 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Hi

<snip>

>> Can I suggest the following logic:
>>
>> 1. if the object already implements the IIndexableObject marker
>> interface, no wrapping is required;
>>
>> 2. otherwise, adapt to IIndexableObject to do the wrapping;
>>
>> 3. if no adapter is registered, fall back to the existing
>> IndexableObjectWrapper class for BBB.
>
> That sounds like what I had in mind, but not for BBB. I think of the
> adapter scheme as a way to choose a non-default wrapper, rather than a
> quasi-mandatory replacement for it.

Ok, well this logic is checked in now on the branch, and tests adjusted
accordingly. Without any warnings.

Miles

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

tseaver at palladion

Mar 18, 2009, 7:06 AM

Post #57 of 60 (1147 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

yuppie wrote:
> Hi!
>
>
> Tres Seaver wrote:
>> Miles wrote:
>>> >> Can I suggest the following logic:
>>> >>
>>> >> 1. if the object already implements the IIndexableObject marker
>>> >> interface, no wrapping is required;
>
> If we don't support 3., we can make 'no wrapping' the default.
>
>>> >> 2. otherwise, adapt to IIndexableObject to do the wrapping;
>>> >>
>>> >> 3. if no adapter is registered, fall back to the existing
>>> >> IndexableObjectWrapper class for BBB.
>>> >
>>> > That sounds like what I had in mind, but not for BBB. I think of the
>>> > adapter scheme as a way to choose a non-default wrapper, rather than a
>>> > quasi-mandatory replacement for it.
>
> What's the win of providing a default that way? IndexableObjectWrapper
> contains policy decisions, Plone e.g. doesn't use it. The current code
> on the branch registers an adapter for IContentish, so CMFDefault will
> never use that hardcoded default.

I think that registration should be in CMFDefault, anyway. Applications
which haven't updated to this new model should continue to work
(including indexing 'allowedRolesAndUsers').

> The change is in a new feature release. People can't expect full BBB if
> they use customized registrations or catalog content that doesn't
> implement IContentish.
>
>>> Ok, well this logic is checked in now on the branch, and tests adjusted
>>> accordingly. Without any warnings.
>> Thanks, looks good.
>
> Looks unnecessarily complex to me. But I'm afraid I'm outvoted.

Falling back to the current behavior is cheap, both at runtime and in
maintenance costs. Why break BBB gratuitously?

Tres.
- --
===================================================================
Tres Seaver +1 540-429-0999 tseaver [at] palladion
Palladion Software "Excellence by Design" http://palladion.com
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.6 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFJwP/e+gerLs4ltQ4RAmr3AJ9vBrqTWDyDo/DSPGjY00p6XIJbvQCgpTW1
9puaqcux8hIdow0DLsFZ4i4=
=VeKx
-----END PGP SIGNATURE-----

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

miles at jamkit

Mar 18, 2009, 7:52 AM

Post #58 of 60 (1154 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Hello

> Tres Seaver wrote:
>> Miles wrote:
>>> >> Can I suggest the following logic:
>>> >>
>>> >> 1. if the object already implements the IIndexableObject marker
>>> >> interface, no wrapping is required;
>
> If we don't support 3., we can make 'no wrapping' the default.

I think this is a change in the way the catalog works - no wrapping is
not the default at the moment, and I can imagine some "hard to identify"
bugs appearing if it did change.

>>> >> 2. otherwise, adapt to IIndexableObject to do the wrapping;
>>> >>
>>> >> 3. if no adapter is registered, fall back to the existing
>>> >> IndexableObjectWrapper class for BBB.
>>> >
>>> > That sounds like what I had in mind, but not for BBB. I think of the
>>> > adapter scheme as a way to choose a non-default wrapper, rather than a
>>> > quasi-mandatory replacement for it.
>
> What's the win of providing a default that way? IndexableObjectWrapper
> contains policy decisions, Plone e.g. doesn't use it. The current code
> on the branch registers an adapter for IContentish, so CMFDefault will
> never use that hardcoded default.
>
> The change is in a new feature release. People can't expect full BBB if
> they use customized registrations or catalog content that doesn't
> implement IContentish.
>
>>> Ok, well this logic is checked in now on the branch, and tests adjusted
>>> accordingly. Without any warnings.
>> Thanks, looks good.
>
> Looks unnecessarily complex to me. But I'm afraid I'm outvoted.

I'm all for providing a fallback so existing "unknown" sites work well
and the behaviour doesn't change, but to be honest I agree with yuppie -
it just seems a bit crufty to be there "forever".

Thinking about it, I'd be concerned that it would mask real errors:
Consider if your adapter registration isn't firing for some reason; your
objects appear to be being cataloged just fine and it's only weeks later
the problem comes to light that a subtly different wrapper was being used.

I think either we provide no fallback (i.e. step 3 doesn't exist, it
just errors) or it spits out a warning when it uses step 3. Generally,
I think the ultimate aim should be just using adapter registrations for
this sort of behaviour.

Miles

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

y.2009 at wcm-solutions

Mar 18, 2009, 8:26 AM

Post #59 of 60 (1158 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Hi!

Tres Seaver wrote:
> Falling back to the current behavior is cheap, both at runtime and in
> maintenance costs. Why break BBB gratuitously?

Because this would be a simple *generic* solution:

w = queryMultiAdapter((obj, self), IIndexableObject, default=obj)

That code could be pushed down the stack to ZCatalog and CMF could use
that feature by registering a CMF-specific adapter. No need to override
the catalog_object method in CatalogTool.

And I doubt missing BBB would hurt many people: If you don't include the
ZCML files from CMF, you have to update your registrations anyway. And
who catalogs content that doesn't implement IContentish?

Cheers,

Yuppie

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

miles at jamkit

Mar 23, 2009, 4:20 AM

Post #60 of 60 (1104 views)
Permalink

Re: IndexableObjectWrapper [In reply to]

Hi,

Just to confirm that I merged this change into the trunk.
http://svn.zope.org/Products.CMFCore/trunk/Products/CMFCore/?rev=98305&view=rev

Miles

Miles wrote:
> Hi,
>
> Can anyone tell me if it is possible to register an adapter to provide a
> different IndexableObjectWrapper class from the stock CMF one? There's
> some sort of framework code that hints that it ought to enable this, but
> I can't see how!
>
> The code still seems to instantiate the wrapper directly using the
> class, rather than looking it up via the component architecture.
>
> Can anyone explain what's going on? I've drawn a blank from googling
> for examples.
>
> Thanks,
>
> Miles
>
>
>
> _______________________________________________
> Zope-CMF maillist - Zope-CMF [at] lists
> http://mail.zope.org/mailman/listinfo/zope-cmf
>
> See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests
>

_______________________________________________
Zope-CMF maillist - Zope-CMF [at] lists
http://mail.zope.org/mailman/listinfo/zope-cmf

See https://bugs.launchpad.net/zope-cmf/ for bug reports and feature requests

1 2 3

View All

Index | Next | Previous | View Threaded

Interested in having your list archived? Contact Gossamer Threads