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

Modernize request parameter handling #641

Open
d-maurer opened this issue May 28, 2019 · 2 comments
Open

Modernize request parameter handling #641

d-maurer opened this issue May 28, 2019 · 2 comments

Comments

@d-maurer
Copy link
Contributor

d-maurer commented May 28, 2019

Zope's current request parameter handling has several known bugs/caveats:

  1. There is almost no error handling:

    • unrecognised directives are silently ignored
    • if a request parameter contains several converter directives, the
      leftmost wins
    • if a request parameter contains several encoding directives, the
      leftmost wins
    • if a request parameter contains an encoding but no converter
      directive, the encoding directive is silently ignored
    • some directive combinations do not make sense (e.g. :record:records);
      for them, some of the directives are silently ignored
  2. Usually, the order of aggregator directives in a request parameter does
    not matter. However, this is not the case for the :tuple directive.
    To really produce a tuple request variable, it must be the left most
    directive; otherwise, it is equivalent to :list.

    In addition, :tuple is always equivalent to :list for
    request variables aggregated as record or sequence of records.

  3. The main use case for the :default directive is to provide a
    default value for form controls (e.g. checkboxes) for which the browser may
    or may not pass on a value when the form is submitted.
    Unfortunately, this only works at the top level.
    It does not work for subcomponents, e.g. an attribute of a "record".
    As a consequence, if a request parameter combines :default with
    another aggregator directive, the result may be unexpected.

  4. The request preprocessing happens at a very early stage, before
    traversal has taken place. As a consequence,
    important configuration for application specific error handling
    may not yet have taken effect. Exceptions raised during this stage
    are reported and tracked only via "root level" error handling.

In addition, for Python 3, Zope's handling of encoding directives is currently broken (--> #634) and converter support is partially broken (#638, #558). Furthermore, Zope's encoding support is not in line with modern standards, e.g. HTML5 form support.

This issue proposes a major overhaul of Zope's request parameter handling:

  • either drop support for the zpublisher-default-encoding configuration option and fix the encoding to utf-8 (this seems to be in line with current standards, see e.g. HTML living standard; they seem to view any non-utf-8 encoding as "legacy support" only) or fully support
    HTML5' form encoding practices.
  • replace the current (apparently ad hoc) handling of aggregator directives by a consistent, orthogonal, fully recursive implementation with clear semantics (thus, hopefully, avoiding surprises as with the current :default and :tuple aggregator)
  • overhaul the implementation for "converter" and "encoding" directives; in particular ensure that for Python 3, the "u*" converter directives behave as the converter directive without the "u" prefix; view the "encoding" as parameter for the "converter"
  • fully recognize errors but delay error reporting by registering an appropriate "PostAuthenticationHook"; this way errors can in many cases (when the error does not affect the following traversal) be reported in the correct application context.
  • support the special handling of HTML's input controls of type image.
  • (maybe) support jQuery's "modern" parameter serialization (apparently initially invented by PHP and now also use by Ruby on rails): i.e. var[] is equivalent to var:list and var[attr] to var.attr:record.
@icemac
Copy link
Member

icemac commented Jun 12, 2019

@d-maurer wrote:
[…]

This issue proposes a major overhaul of Zope's request parameter handling:

First of all, I personally do not use Zope's request parameters, but I see the issues and the need for them to be fixed.

  • either drop support for the zpublisher-default-encoding configuration option and fix the encoding to utf-8 (this seems to be in line with current standards, see e.g. HTML living standard; they seem to view any non-utf-8 encoding as "legacy support" only) or fully support HTML5' form encoding practices.

Dropping zpublisher-default-encoding configuration option would make it for older code using latin1 though out the whole application more difficult to migrate. So I'd like to keep it as it is.

  • replace the current (apparently ad hoc) handling of aggregator directives by a consistent, orthogonal, fully recursive implementation with clear semantics (thus, hopefully, avoiding surprises as with the current :default and :tuple aggregator)

+1

  • overhaul the implementation for "converter" and "encoding" directives; in particular ensure that for Python 3, the "u*" converter directives behave as the converter directive without the "u" prefix; view the "encoding" as parameter for the "converter"

+1

  • fully recognize errors but delay error reporting by registering an appropriate "PostAuthenticationHook"; this way errors can in many cases (when the error does not affect the following traversal) be reported in the correct application context.

+1

  • support the special handling of HTML's input controls of type image.

good idea

  • (maybe) support jQuery's "modern" parameter serialization (apparently initially invented by PHP and now also use by Ruby on rails): i.e. var[] is equivalent to var:list and var[attr] to var.attr:record.

I am not sure if there will be someone using these new spellings, especially as the old ones should be kept.

@icemac icemac added this to the 5.0 milestone Jun 12, 2019
@icemac icemac modified the milestones: 5.0, 5.1 Jun 12, 2019
@d-maurer
Copy link
Contributor Author

@icemac

@d-maurer wrote:
...

  • (maybe) support jQuery's "modern" parameter serialization (apparently initially invented by PHP and now also use by Ruby on rails): i.e. var[] is equivalent to var:list and var[attr] to var.attr:record.

I am not sure if there will be someone using these new spellings, especially as the old ones should be kept.

It is used by jQuery's params (unless you specify traditional mode) when you call it with a JS object containing more complex values (such as lists or subobjects).
But, you are right. It would make an already complex implementation even more complex.

d-maurer added a commit that referenced this issue Jun 12, 2019
Tests broken due to backward incompatible changes not yet fixed
@icemac icemac modified the milestones: 5.1, 5.2 Apr 20, 2021
@icemac icemac modified the milestones: 5.2, 5.4 Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants