-
-
Notifications
You must be signed in to change notification settings - Fork 42
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
Use INameFromTitle
interface from plone.base
#379
Conversation
@gforcada thanks for creating this Pull Request and helping to improve Plone! TL;DR: Finish pushing changes, pass all other checks, then paste a comment:
To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically. Happy hacking! |
I have released a new |
Actually, |
Failure seems related:
I did not dive in, and probably won't do that today. @gforcada ? |
@mauritsvanrees @gforcada I tried to take a look at this, but the tests pass for me in buildout.coredev, and when I try
|
@davisagli The tests pass for me in coredev 6.1, but not in 6.0. Of course one thing that still needs to happen, is that this needs to be tested in combination with plone/plone.app.content#271. But even then, when I run the 6.0 tests with both branches checked out, it fails in the same way as here in gh-actions. It seems to work fine in practice though: when I fire up an instance and create a new Image and a new Page, they both get a normalised id based on the file name or the page title. So I wonder if something is wrong in test setup, that some zcml is not loading, but I don't see what it would be. |
Okay, I kind-of see what is going on, but not completely. In The way to fix this in the test, is to use the named behavior But the problem then remains: if there is a site that uses this behavior by interface, either the old or the new one, then it fails. Maybe someone can come up with a combination that works, but I could not. I think we need to postpone this PR and plone/plone.app.content#271 until at least Plone 6.1, and maybe 7.0. What might help, is one of the following, probably somewhere in
Wait, would
Ah but this is used in |
@gforcada @davisagli We may need to revert the I would be happy to be shown an error in my logic. |
This is for `INameFromTitle`, which we want to move to `plone.base`. See #379 (comment) for why this is not working yet, and we need this workaround.
This is for `INameFromTitle`, which we want to move to `plone.base`. See #379 (comment) for why this is not working yet, and we need this workaround.
This is for `INameFromTitle`, which we want to move to `plone.base`. See #379 (comment) for why this is not working yet, and we need this workaround.
I have created a PR to make For the moment in Plone 6.0.8 we could revert |
@mauritsvanrees I think you are on the right track. Earlier I missed that the old interface location could be stored somewhere persistently. There isn't much harm in leaving the INameFromTitle in plone.base, except for potential confusion. We could leave it there and update INameFromTitle in plone.app.content to extend it, so that we're at least moving in the intended direction. |
Closed in favour of PR #380. |
Branch: refs/heads/master Date: 2023-10-15T17:57:43+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.app.content@85a904c feat: deprecate INameFromTitle in this distribution It got moved to `plone.base`. Adds a deprecation warning so the interface can be moved right away, but does not break other code that still imports from here, and show a deprecation warning about it. Files changed: M plone/app/content/interfaces.py M plone/app/content/namechooser.py M plone/app/content/namechooser.txt Repository: plone.app.content Branch: refs/heads/master Date: 2023-10-15T17:59:36+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.app.content@de83c82 Add news entry Files changed: A news/3858.internal.2 Repository: plone.app.content Branch: refs/heads/master Date: 2023-10-25T17:18:41+02:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.content@9a9dbf3 Merge branch 'master' into use-interface-from-plone-base Files changed: A .editorconfig A .flake8 A .github/workflows/meta.yml A .meta.toml A .pre-commit-config.yaml A tox.ini M .gitignore M CHANGES.rst M plone/app/content/browser/actions.py M plone/app/content/browser/configure.zcml M plone/app/content/browser/constraintypes.pt M plone/app/content/browser/constraintypes.py M plone/app/content/browser/content_status_history.py M plone/app/content/browser/contents/__init__.py M plone/app/content/browser/contents/configure.zcml M plone/app/content/browser/contents/copy.py M plone/app/content/browser/contents/cut.py M plone/app/content/browser/contents/delete.py M plone/app/content/browser/contents/paste.py M plone/app/content/browser/contents/properties.py M plone/app/content/browser/contents/rename.py M plone/app/content/browser/contents/tags.py M plone/app/content/browser/contents/workflow.py M plone/app/content/browser/folderfactories.pt M plone/app/content/browser/full_review_list.pt M plone/app/content/browser/i18n.py M plone/app/content/browser/interfaces.py M plone/app/content/browser/selection.py M plone/app/content/browser/table.pt M plone/app/content/browser/table.txt M plone/app/content/browser/templates/content_status_history.pt M plone/app/content/browser/templates/delete_confirmation.pt M plone/app/content/browser/templates/object_rename.pt M plone/app/content/browser/templates/select_default_page.pt M plone/app/content/browser/templates/select_default_view.pt M plone/app/content/browser/vocabulary.py M plone/app/content/configure.zcml M plone/app/content/namechooser.py M plone/app/content/testing.py M plone/app/content/tests/profiles/non-ascii-workflow-profile/workflows.xml M plone/app/content/tests/profiles/non-ascii-workflow-profile/workflows/non-ascii-workflow/definition.xml M plone/app/content/tests/profiles/non-ascii-workflow.zcml M plone/app/content/tests/test_actions.py M plone/app/content/tests/test_adding.py M plone/app/content/tests/test_content_status_modify.py M plone/app/content/tests/test_contents.py M plone/app/content/tests/test_folder.py M plone/app/content/tests/test_namechooser_unit.py M plone/app/content/tests/test_non_ascii_characters_in_workflow_state.py M plone/app/content/tests/test_selectdefaultpage.py M plone/app/content/tests/test_widgets.py M plone/app/content/utils.py M pyproject.toml M setup.py D news/266.bugfix D news/268.bugfix D news/3858.internal D setup.cfg Repository: plone.app.content Branch: refs/heads/master Date: 2023-10-31T22:08:10+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.content@ccc85f6 Remove deprecation warning for INameFromTitle as it breaks some use cases. See plone/plone.app.dexterity#379 (comment) Files changed: A news/3858.internal M plone/app/content/interfaces.py D news/3858.internal.2 Repository: plone.app.content Branch: refs/heads/master Date: 2023-11-02T11:54:18+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.content@d487863 Merge pull request #271 from plone/use-interface-from-plone-base Move `INameFromTitle` interface to `plone.base` Files changed: A news/3858.internal M plone/app/content/interfaces.py M plone/app/content/namechooser.py M plone/app/content/namechooser.txt
Branch: refs/heads/master Date: 2023-10-15T17:57:43+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.app.content@85a904c feat: deprecate INameFromTitle in this distribution It got moved to `plone.base`. Adds a deprecation warning so the interface can be moved right away, but does not break other code that still imports from here, and show a deprecation warning about it. Files changed: M plone/app/content/interfaces.py M plone/app/content/namechooser.py M plone/app/content/namechooser.txt Repository: plone.app.content Branch: refs/heads/master Date: 2023-10-15T17:59:36+02:00 Author: Gil Forcada Codinachs (gforcada) <gil.gnome@gmail.com> Commit: plone/plone.app.content@de83c82 Add news entry Files changed: A news/3858.internal.2 Repository: plone.app.content Branch: refs/heads/master Date: 2023-10-25T17:18:41+02:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.content@9a9dbf3 Merge branch 'master' into use-interface-from-plone-base Files changed: A .editorconfig A .flake8 A .github/workflows/meta.yml A .meta.toml A .pre-commit-config.yaml A tox.ini M .gitignore M CHANGES.rst M plone/app/content/browser/actions.py M plone/app/content/browser/configure.zcml M plone/app/content/browser/constraintypes.pt M plone/app/content/browser/constraintypes.py M plone/app/content/browser/content_status_history.py M plone/app/content/browser/contents/__init__.py M plone/app/content/browser/contents/configure.zcml M plone/app/content/browser/contents/copy.py M plone/app/content/browser/contents/cut.py M plone/app/content/browser/contents/delete.py M plone/app/content/browser/contents/paste.py M plone/app/content/browser/contents/properties.py M plone/app/content/browser/contents/rename.py M plone/app/content/browser/contents/tags.py M plone/app/content/browser/contents/workflow.py M plone/app/content/browser/folderfactories.pt M plone/app/content/browser/full_review_list.pt M plone/app/content/browser/i18n.py M plone/app/content/browser/interfaces.py M plone/app/content/browser/selection.py M plone/app/content/browser/table.pt M plone/app/content/browser/table.txt M plone/app/content/browser/templates/content_status_history.pt M plone/app/content/browser/templates/delete_confirmation.pt M plone/app/content/browser/templates/object_rename.pt M plone/app/content/browser/templates/select_default_page.pt M plone/app/content/browser/templates/select_default_view.pt M plone/app/content/browser/vocabulary.py M plone/app/content/configure.zcml M plone/app/content/namechooser.py M plone/app/content/testing.py M plone/app/content/tests/profiles/non-ascii-workflow-profile/workflows.xml M plone/app/content/tests/profiles/non-ascii-workflow-profile/workflows/non-ascii-workflow/definition.xml M plone/app/content/tests/profiles/non-ascii-workflow.zcml M plone/app/content/tests/test_actions.py M plone/app/content/tests/test_adding.py M plone/app/content/tests/test_content_status_modify.py M plone/app/content/tests/test_contents.py M plone/app/content/tests/test_folder.py M plone/app/content/tests/test_namechooser_unit.py M plone/app/content/tests/test_non_ascii_characters_in_workflow_state.py M plone/app/content/tests/test_selectdefaultpage.py M plone/app/content/tests/test_widgets.py M plone/app/content/utils.py M pyproject.toml M setup.py D news/266.bugfix D news/268.bugfix D news/3858.internal D setup.cfg Repository: plone.app.content Branch: refs/heads/master Date: 2023-10-31T22:08:10+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.content@ccc85f6 Remove deprecation warning for INameFromTitle as it breaks some use cases. See plone/plone.app.dexterity#379 (comment) Files changed: A news/3858.internal M plone/app/content/interfaces.py D news/3858.internal.2 Repository: plone.app.content Branch: refs/heads/master Date: 2023-11-02T11:54:18+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.content@d487863 Merge pull request #271 from plone/use-interface-from-plone-base Move `INameFromTitle` interface to `plone.base` Files changed: A news/3858.internal M plone/app/content/interfaces.py M plone/app/content/namechooser.py M plone/app/content/namechooser.txt
Branch: refs/heads/master Date: 2023-10-31T20:50:53+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.dexterity@e9466e8 Make the dependency on plone.app.content conditional. This is for `INameFromTitle`, which we want to move to `plone.base`. See plone/plone.app.dexterity#379 (comment) for why this is not working yet, and we need this workaround. Files changed: A news/3858.internal M .meta.toml M plone/app/dexterity/behaviors/configure.zcml M pyproject.toml M setup.py Repository: plone.app.dexterity Branch: refs/heads/master Date: 2023-10-31T22:17:23+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.dexterity@d4ebbe1 Test that both spellings of the plone.namefromtitle behavior work. Both `plone.namefromtitle` and `plone.app.content.interfaces.INameFromTitle` work. You cannot use `plone.base.interfaces.INameFromTitle` as behavior name, this was never supported. Recommended is to use `plone.namefromtitle` in all supported Plone versions. Using `plone.app.content.interfaces.INameFromTitle` as name will stop working in Plone 7. Rewrote the `tests/namefromtitle.txt` doctest file as a unittest file, making it easier to test both behavior names. Files changed: A plone/app/dexterity/tests/test_namefromtitle.py M plone/app/dexterity/behaviors/configure.zcml M plone/app/dexterity/tests/test_doctests.py D plone/app/dexterity/tests/namefromtitle.txt Repository: plone.app.dexterity Branch: refs/heads/master Date: 2023-11-02T11:54:24+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.dexterity@669e1e7 Merge pull request #380 from plone/maurits-plone-app-content-conditional Make the dependency on plone.app.content conditional. Files changed: A news/3858.internal A plone/app/dexterity/tests/test_namefromtitle.py M .meta.toml M plone/app/dexterity/behaviors/configure.zcml M plone/app/dexterity/tests/test_doctests.py M pyproject.toml M setup.py D plone/app/dexterity/tests/namefromtitle.txt
Branch: refs/heads/master Date: 2023-10-31T20:50:53+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.dexterity@e9466e8 Make the dependency on plone.app.content conditional. This is for `INameFromTitle`, which we want to move to `plone.base`. See plone/plone.app.dexterity#379 (comment) for why this is not working yet, and we need this workaround. Files changed: A news/3858.internal M .meta.toml M plone/app/dexterity/behaviors/configure.zcml M pyproject.toml M setup.py Repository: plone.app.dexterity Branch: refs/heads/master Date: 2023-10-31T22:17:23+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.dexterity@d4ebbe1 Test that both spellings of the plone.namefromtitle behavior work. Both `plone.namefromtitle` and `plone.app.content.interfaces.INameFromTitle` work. You cannot use `plone.base.interfaces.INameFromTitle` as behavior name, this was never supported. Recommended is to use `plone.namefromtitle` in all supported Plone versions. Using `plone.app.content.interfaces.INameFromTitle` as name will stop working in Plone 7. Rewrote the `tests/namefromtitle.txt` doctest file as a unittest file, making it easier to test both behavior names. Files changed: A plone/app/dexterity/tests/test_namefromtitle.py M plone/app/dexterity/behaviors/configure.zcml M plone/app/dexterity/tests/test_doctests.py D plone/app/dexterity/tests/namefromtitle.txt Repository: plone.app.dexterity Branch: refs/heads/master Date: 2023-11-02T11:54:24+01:00 Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org> Commit: plone/plone.app.dexterity@669e1e7 Merge pull request #380 from plone/maurits-plone-app-content-conditional Make the dependency on plone.app.content conditional. Files changed: A news/3858.internal A plone/app/dexterity/tests/test_namefromtitle.py M .meta.toml M plone/app/dexterity/behaviors/configure.zcml M plone/app/dexterity/tests/test_doctests.py M pyproject.toml M setup.py D plone/app/dexterity/tests/namefromtitle.txt
This is part of plone/Products.CMFPlone#3858
It needs plone/plone.base#51 to be merged, and released.
It first needs #378 to be merged first.
See the tracking issue for the overview of the changes and why.