-
-
Notifications
You must be signed in to change notification settings - Fork 75
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
Checkout Zope and use its master versions. #844
Conversation
|
de321fd
to
03bf5d9
Compare
I rebased, squashed, and force pushed. Should work now. @jenkins-plone-org please run jobs |
I would say, merge yourself if ready. |
In all jobs 19 failing tests. Really? :-( |
see my condition - I would not call it ready ;) |
looks like the signature changed and the test dummy no longer works https://github.com/zopefoundation/Zope/blob/master/src/ZPublisher/HTTPRequest.py#L1486 commit introducing this is |
We could change the signature in Zope master to |
Without this change, some tests (and possibly live code) in Plone fail because they do not pass the new required `charset` parameter. See plone/buildout.coredev#844 And see discussion that I just started here: #1094 (comment) Also fixes an unclosed file in a test.
Without this change, some tests (and possibly live code) in Plone fail because they do not pass the new required `charset` parameter. See plone/buildout.coredev#844 And see discussion that I just started here: #1094 (comment) Also fixes an unclosed file in a test.
03bf5d9
to
bd33f98
Compare
There are several major updates there, which should be fine: they drop Python 2 support. I do see that several pins have been dropped, so we need to add more ourselves. This is the output when allowing unpinned versions: ``` mock = 5.0.1 zope.sendmail = 5.3 ZEO = 5.4.0 future = 0.18.3 zdaemon = 4.4 zope.componentvocabulary = 2.3.0 zope.ramcache = 2.4 ``` Pinned several others as well that used to be pinned by Zope. They are not getting pulled in on Python 3.11 on my Mac, so hard to say where these dependencies comes from.
This part can be revert once these two PRs are merged: - zopefoundation/Zope#1101 - plone/plone.formwidget.namedfile#60
bd33f98
to
cb0b066
Compare
@jenkins-plone-org please run jobs |
Three robot tests fail. They are the same across all four Python versions. TLDR: I found the problem and it needs a one-line fix in Zope. I looked at this one and I have the same test failure locally and can reproduce it in an actual site:
That is on this line:
I traced this back to Zope: if I go to @d-maurer, that is your PR. Do you have an idea what causes this? On a (POST) request where reordering works, I have this:
On a request where reordering fails, I get:
So when it fails, the request has a BODY and no form. Ah, found it! It is this line from your PR:
In my case I have this:
So the condition does not match. It should be:
Then reordering works, and the test passes. @d-maurer does that make sense? |
Maurits van Rees wrote at 2023-3-8 15:48 -0800:
...
...
Ah, found it! It is [this line from your PR](https://github.com/zopefoundation/Zope/pull/1094/files#diff-2ac5224ea2051bd5c4328e82ba1a4945abcb3f400e8d59e3cd53e7db074d0eafR1746):
```
elif content_type == "application/x-www-form-urlencoded":
```
In my case I have this:
```
(Pdb++) content_type
'application/x-www-form-urlencoded; charset=UTF-8'
```
So the condition does not match. It should be:
```
elif content_type.startswith("application/x-www-form-urlencoded"):
```
Then reordering works, and the test passes.
@d-maurer does that make sense?
In principle, yes. **BUT** `application/x-www-form-urlencoded`
is defined by "https://www.w3.org/TR/2014/REC-html5-20141028/iana.html":
12.4 application/x-www-form-urlencoded
This registration has been filed successfully with IANA.
Type name:
application
Subtype name:
x-www-form-urlencoded
Required parameters:
No parameters
Optional parameters:
No parameters
This means: `application/x-www-form-urlencoded` is expected
to have **no** parameters - and especially no `charset` parameter.
For that reason, I used "==" instead of `startswith`.
I can make a small PR, but I am getting sleepy, so that will have to wait a bit.
Not sure whether we should make Zope support the use of
a parameter not supported by the relevant standard and
which it would not interpret.
I suggest, you make the tests standard conform, instead.
Maybe also get my other [PR in the same area](zopefoundation/Zope#1101) approved and merged first.
dG>
Sorry: I had requested changes to this PR. I do not know why it was
not listed (maybe, I forgot the "submit"). I have redone the change request
(this time successful).
|
@d-maurer wrote:
Interesting. Most others in that document allow a charset parameter, but indeed this one not.
This is not only in tests. I see this in an actual browser: it makes reordering in folder contents fail. Okay, the folder contents code in I see a few more though, at least these with a simple grep, there may be more:
For So: I could update |
…1101) FileUpload: use default encoding as charset when nothing is passed. Without this change, some tests (and possibly live code) in Plone fail because they do not pass the new required `charset` parameter. See plone/buildout.coredev#844 And see discussion that I just started here: #1094 (comment) Also fixes an unclosed file in a test.
Maurits van Rees wrote at 2023-3-9 01:11 -0800:
...
But I do wonder if it makes sense to make the Zope code more lenient, to avoid problems for other code out there that makes the same mistake.
If we allow the parameter, its value should get interpreted, too.
However, this would require significantly more work than
the replacement of `==` by `startswith`.
In fact, the current `multipart` integration into `HTTPRequest.processInputs`
is based on the assumption that `charset` information comes either
from `default_encoding` or via explicit conversion specs in the form variable
names.
|
Branch maurits-fileupload-charset-latin-1 was merged.
Dieter Maurer wrote at 2023-3-9 10:24 +0100:
Maurits van Rees wrote at 2023-3-9 01:11 -0800:
> ...
> But I do wonder if it makes sense to make the Zope code more lenient, to avoid problems for other code out there that makes the same mistake.
If we allow the parameter, its value should get interpreted, too.
However, this would require significantly more work than
the replacement of `==` by `startswith`.
In fact, the current `multipart` integration into `HTTPRequest.processInputs`
is based on the assumption that `charset` information comes either
from `default_encoding` or via explicit conversion specs in the form variable
names.
The server usually uses `default_encoding` as charset for its forms.
Therefore, it can expect that received form data by default uses
this charset as well.
If form data arrives with an explicit `charset`, then the client
likely wants to tell the server that it has used this `charset` when
it constructed the form data. This would mean that the
server should use this explicit charset (rather than its `default_encoding`)
as default during the processing of this form data.
This suggests how a `charset` parameter should be supported by `Zope`
(if it is supported at all).
Alternative: raise an exception when the unsupported `charset` parameter
is encountered.
|
…ncoded header. Adding a charset to this Content-Type is not supported by the definition, and fails with Zope master. See plone/buildout.coredev#844 (comment)
Adding a charset to this Content-Type is illegal according to the definition, and fails with Zope master. See plone/buildout.coredev#844 (comment) In our case, when you click to add a recurrence, you get a dialog box saying 'error' because the call to `@@json_recurrence` failed.
I made two PRs to remove the charset:
Those fix a few real problems in the folder contents and for recurring events. But reordering in the folder contents still does not work. And I have no idea which code is responsible for adding the Content-Type header @thet @petschki Something in the folder contents code results in a Content-Type header @d-maurer What about accepting the charset in the header when it is the same as the |
Warn when this does not fit the default encoding. This is my proposal from plone/buildout.coredev#844 (comment) It fixes several problems in Plone.
@jenkins-plone-org please run jobs |
Maurits van Rees wrote at 2023-3-9 13:25 -0800:
...
@d-maurer What about accepting the charset in the header when it is the same as the `default_encoding`? We would still ignore it, but at least we would use the wanted code path.
When the charset does not match, we could log a warning and then ignore it.
I would not be happy.
Should you really be unable to get rid of the `charset` parameter
for the `application/x-www-form-urlencoded` media type,
I think that Zope should support the parameter fully.
This will only be slightly more complex than what you have
proposed above:
currently, the initial request data processing returns a `FieldStorage`;
to fully support the `charset` parameter, it would instead
return a tuple `FieldStorage, charset`. If `charset` is not `None`,
it is used (instead of `default_encoding`) as the default encoding
for the current form data interpretation.
Everything else can mostly remain as it is now (we might need
to introduce a new local variable to replace `default_encoding` locally).
|
See my comment here plone/mockup#1292 (comment) |
So all/most jQuery Ajax calls are the problem! Thanks for figuring that out @petschki. At least with all the various PRs included, the jobs pass, so we are in the right direction. |
Use the standard branches from mockup and plone.formwidget.recurrence again. Although my fixes on the previous branches are fine, they should not be needed anymore.
With Dieter's branch from zopefoundation/Zope#1104 both the folder contents and recurring events work again. Thanks! Let's see if Jenkins is happy too: @jenkins-plone-org please run jobs |
Branch: refs/heads/master Date: 2023-03-09T22:00:47+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/plone.formwidget.recurrence@25d552c Do not add charset to application/x-www-form-urlencoded header. Adding a charset to this Content-Type is illegal according to the definition, and fails with Zope master. See #844 (comment) In our case, when you click to add a recurrence, you get a dialog box saying 'error' because the call to `@@json_recurrence` failed. Files changed: A news/844.bugfix M plone/formwidget/recurrence/tests/test_z3cwidget.py M plone/formwidget/recurrence/z3cform/widget.py Repository: plone.formwidget.recurrence Branch: refs/heads/master Date: 2023-03-10T21:31:21+01:00 Author: Maurits van Rees (mauritsvanrees) <[email protected]> Commit: plone/plone.formwidget.recurrence@25e1695 Merge pull request #31 from plone/maurits-no-charset-with-x-www-form-urlencoded Do not add charset to application/x-www-form-urlencoded header. Files changed: A news/844.bugfix M plone/formwidget/recurrence/tests/test_z3cwidget.py M plone/formwidget/recurrence/z3cform/widget.py
All is well now. I merged this manually, plus the related plone.formwidget.namedfile/recurrence PRs. |
There are several major updates there, which should be fine: they drop Python 2 support.
I do see that several pins have been dropped, so we need to add more ourselves. This is the output when allowing unpinned versions:
I added them.