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

Remove deprecated lookup of behavior interface. #169

Open
mauritsvanrees opened this issue Jul 1, 2022 · 8 comments
Open

Remove deprecated lookup of behavior interface. #169

mauritsvanrees opened this issue Jul 1, 2022 · 8 comments

Comments

@mauritsvanrees
Copy link
Member

If behavior lookup by name fails, we fall back to trying to resolve the name as an interface.
The code has this comment:

BBB - this case should be deprecated in v 3.0

This was added 8 years ago in this commit as part of PR #21, which ended up in version 2.3.0.
Later, the deprecation warning was updated in PR #95, in 2.9.0, to mention that the fallback will be removed in 3.0.

So:

  • There was a BBB comment since 2.3.0, which is Plone 5.0.0.
  • It was more clearly deprecated since 2.9.0, which is Plone 5.2.0.
  • Version 3 is for Plone 6 only.

So we could remove this, which we should do before we hit beta.

Do we indeed still want to remove this?
I probably have lots of behaviors in client code that still do not have a name. Or they have a name, but it is not used in the portal_types definition in the database.
Having only one way to spell behaviors would be good though, and now would be the time.

@jensens Do you know if the fallback is significantly slower than the named lookup? Of course less code is faster, but if it is just a microsecond extra, then it is not so bad. It does look more expensive.

Is there code already to migrate existing portal_types? Ah, plone.app.upgrade has it. Looks like this was called for one of the first 5.2 release candidates. We could edit this to show a warning when named lookup fails. And run it again in Plone 6 for good measure. Maybe add code to update a single portal_type, which could then be used by add-ons in their own upgrade step.

@jensens
Copy link
Member

jensens commented Jul 1, 2022

+1000 to remove the code.

Speed is probably now the major point here, moreover a name has other advantages than a dotted path. But getting rid of the fallback may give some micro-secs. And - without profiling here - removing a bunch of registration from the adapter registry may speed it up as well a tiny bit.

Anyway, there should be no code around in the wild anymore using dotted paths. Except if one ignored deprecation messages for many years.

@jensens
Copy link
Member

jensens commented Jul 1, 2022

If we remove the fallback here, we can also check plone.behavior registration in https://github.com/plone/plone.behavior/blob/master/plone/behavior/metaconfigure.py

name_only could be set to True by default.

@mauritsvanrees
Copy link
Member Author

name_only could be set to True by default.

Good point.

We could additionally set name to be required: without a name the behavior is not found anymore so it is useless to have a nameless behavior.

I guess GenericSetup export/import code could still do the interface lookup: when a type info xml has behavior example.code.behaviors.IAmOld it could lookup the name and store this name in portal_types.

mauritsvanrees added a commit to plone/plone.app.dexterity that referenced this issue Jul 5, 2022
The 'name' property may become required.
See plone/plone.dexterity#169
mauritsvanrees added a commit to plone/plone.restapi that referenced this issue Jul 5, 2022
The 'name' property may become required.
See plone/plone.dexterity#169
mauritsvanrees added a commit to plone/plone.restapi that referenced this issue Jul 5, 2022
The 'name' property may become required.
See plone/plone.dexterity#169
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jul 5, 2022
Branch: refs/heads/master
Date: 2022-07-05T14:52:39+02:00
Author: Maurits van Rees (mauritsvanrees) <[email protected]>
Commit: plone/plone.app.dexterity@df37fe3

Tests: add names to behaviors.

The 'name' property may become required.
See plone/plone.dexterity#169

Files changed:
A news/169.bugfix
M plone/app/dexterity/textindexer/tests/configure.zcml
Repository: plone.app.dexterity

Branch: refs/heads/master
Date: 2022-07-05T16:21:18+02:00
Author: Jens W. Klein (jensens) <[email protected]>
Commit: plone/plone.app.dexterity@edabfcb

Merge pull request #352 from plone/maurits-behavior-names

Tests: add names to behaviors.

Files changed:
A news/169.bugfix
M plone/app/dexterity/textindexer/tests/configure.zcml
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Jul 5, 2022
Branch: refs/heads/master
Date: 2022-07-05T15:33:54+02:00
Author: Maurits van Rees (mauritsvanrees) <[email protected]>
Commit: plone/plone.restapi@f25f9ba

Tests: add names to behaviors.

The 'name' property may become required.
See plone/plone.dexterity#169

Files changed:
A news/169.bugfix
M src/plone/restapi/profiles/testing/types/DXTestDocument.xml
M src/plone/restapi/testing.zcml
M src/plone/restapi/tests/http-examples/controlpanels_get_dexterity_item.resp
M src/plone/restapi/tests/http-examples/controlpanels_post_dexterity_item.resp
Repository: plone.restapi

Branch: refs/heads/master
Date: 2022-07-05T16:52:54+02:00
Author: Maurits van Rees (mauritsvanrees) <[email protected]>
Commit: plone/plone.restapi@e8eaee3

Merge pull request #1457 from plone/maurits-behavior-names

Tests: add names to behaviors.

Files changed:
A news/169.bugfix
M src/plone/restapi/profiles/testing/types/DXTestDocument.xml
M src/plone/restapi/testing.zcml
M src/plone/restapi/tests/http-examples/controlpanels_get_dexterity_item.resp
M src/plone/restapi/tests/http-examples/controlpanels_post_dexterity_item.resp
@mauritsvanrees
Copy link
Member Author

Might be nicer to automatically set the name to interface.__identifier__ if no explicit name is given. This means less breakage for add-ons. It might defeat the purpose though: I guess best practice is to have an explicit name.

Anyway, if we have an explicit or implicit name, other code can rely on each behavior having a name. For example plone.app.dexterity no longer needs to check this in the controlpanel.

Note that this uses lookup_behavior_registration, which has even more fallback, looking up possible former_dotted_names on all behaviors. It is only used in the plone.app.dexterity controlpanel linked above in this comment, in plone.app.upgrade.v52.final for upgrading to named behaviors, and in plone.app.versioningbehavior.modifiers in CMFEditions-related code. These sound like places where more fallback is useful.

@davisagli
Copy link
Member

I happened to run across plone/plone.app.versioningbehavior#45 which explains why former_dotted_names is needed to support retrieval of old content versions.

@mauritsvanrees
Copy link
Member Author

I doubt whether it is still useful and safe to go through with this. What do we gain?

@jensens
Copy link
Member

jensens commented Jul 9, 2022

We could keep the code and raise an exception unless a special environment variable is set. Then remove it in next major.

@mauritsvanrees
Copy link
Member Author

I have no great need or energy/time to do this myself at this point. That means that if you leave it to me, it probably won't happen before beta, which means it should not happen at all anymore, at least not in 6.0.x.

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

No branches or pull requests

3 participants