Skip to content

#160 - Full control over the launch form fields (type, options, constraints, default) through the new form UI option - #168

Open
clever-age-gtonon wants to merge 1 commit into
mainfrom
#160-execution-form
Open

clever-age-gtonon wants to merge 1 commit into
mainfrom
#160-execution-form

Conversation

@clever-age-gtonon

Copy link
Copy Markdown

Description

Implements #160 (replaces #161, rebased on main): the process configuration now has full control over the UI launch form (ui_launch_mode: form). For the input field and each context key, it declares the form type, its options (with the field constraints) and its default value. Example: the delimiter context value becomes a "Delimiter" select instead of two free text inputs.

options:
    ui:
        ui_launch_mode: form
        form:
            input:
                type: file                 # form type alias or FQCN
                options:
                    label: CSV file
                    constraints:
                        - File:
                              extensions:
                                  csv: ['text/csv', 'text/plain', 'application/csv', 'text/x-csv', 'text/x-comma-separated-values']
            context:
                delimiter:
                    type: choice
                    default: ','
                    options:
                        label: Delimiter
                        choices: { 'Comma (,)': ',', 'Semicolon (;)': ';' }
            allow_extra_context: false     # default true: free key/value pairs for the undeclared keys
            constraints: []                # constraints of the whole {input, context} data (replaces ui.constraints)

The data of the form keeps its shape ({input, context: {key => value}}), so the dispatched message is unchanged.

Changes

New form UI option (ProcessConfigurationsManager):

  • form.input and form.context.<key>: type, options, default.
    • type is a closed list of aliases (text, choice, checkbox, integer, date, datetime, dateinterval, enum, entity…) or a form type FQCN.
    • An unknown type, a class that is not a FormTypeInterface, and a button type are each refused with an explicit InvalidOptionsException.
  • options.constraints uses the same syntax as the global constraints (ConstraintLoader).
  • Guard rails:
    • the data, mapped, property_path and inherit_data options are refused;
    • a file field is refused in the context, and default / multiple are refused on a file input;
    • a context key must be a valid form field name, not only digits;
    • extra and execution_user are reserved keys.
  • form.allow_extra_context (default true): keeps the free key/value collection for the undeclared keys. With false, an undeclared key in the defaults is refused at resolution.
  • form.constraints: constraints of the whole form, as the former constraints.
  • getUiOptions() returns the effective form option: the deprecated format is folded into it.
  • New getLaunchFormDefaults() / buildLaunchFormDefaults().
  • Each getter (getUiOptions, getNotificationOptions, getLogsOptions) now resolves only its own option:
    • an invalid launch form no longer breaks the notifications nor the purge;
    • conversely, see Breaking changes.

Launch form (LaunchType, ProcessContextType, LaunchAction):

  • context is now a compound FormType: one child per declared key, then the extra CollectionType of the free pairs. It is merged back into {key => value} on submit, and the declared keys come first.
    • The violations of form.constraints on [context][key] are mapped to the declared field.
    • required of the declared fields is respected; the rendering of the old format is unchanged.
  • A free pair whose key is a declared field is a validation error: reserved_keys option of ProcessContextType.
  • execution_user is always the email of the connected user:
    • LaunchAction::dispatch() no longer lets the context override it;
    • the key is reserved in the free pairs and in form.context;
    • constant ProcessExecuteMessage::EXECUTION_USER_KEY, constraint ProcessContextType::createExecutionUserConstraint().
  • The key of a free context pair must be a valid name (letters, digits, _, -, :, not only digits): key_constraints option of ProcessContextType. This also applies to the schedules (ProcessScheduleCrudController), which reserve execution_user too.
  • The UI options are resolved once per launch request: new internal ui_options option of LaunchType.
  • New ContextValueNormalizer, applied after the validation. It converts the typed values into process values:
    • scalars are kept, arrays are converted recursively;
    • BackedEnum → value, UnitEnum → name;
    • dates → ATOM, DateInterval → ISO 8601;
    • Stringable → string;
    • files and other objects → LogicException with an explicit message.

Deprecations (removal in 4.0): entrypoint_type → form.input.type, default → form.*.default, constraints → form.constraints.

  • A deprecation is triggered only when the option is set, with the process code and the replacement in the message.
  • Mixing a deprecated option and its replacement raises an InvalidOptionsException.

Dependencies: require symfony/doctrine-bridge (^6.4 || ^7.4 || ^8) for the entity alias.

Documentation:

  • docs/reference/02-process_ui_options.md: form fields, type aliases, value conversion, free pairs, recommendation of allow_extra_context: false when form.context is declared;
  • docs/reference/04-process_executions_and_logs.md, 05-scheduler.md, 09-notifications.md;
  • docs/cookbooks/form_file_upload.md rewritten with form:;
  • UPGRADE.md v3.3 section (before/after example);
  • CHANGELOG.

Tests

403 → 588 tests (1842 assertions), with no deprecation, warning or notice (--display-deprecations --display-warnings --display-notices).

  • ProcessConfigurationsManagerTest:
    • resolution of form: aliases, FQCN, invalid types, constraints, forbidden options, keys, file fields;
    • conflicts and the folding of the old format;
    • exact deprecation messages, triggered only when the option is set;
    • getLaunchFormDefaults;
    • independence of the ui / notification / logs resolutions.
  • ContextValueNormalizerTest (new): every conversion rule and the exceptions.
  • ProcessContextTypeTest: reserved_keys, key_constraints, key format (zz)|x|(?:zz, a/b, a b, {{x}}, 123… refused).
  • LaunchTypeTest:
    • declared fields;
    • merge of the declared and free values;
    • a free pair with a declared key, or with execution_user;
    • required;
    • global constraints mapped to a field;
    • allow_extra_context: false;
    • pre-resolved ui_options.
  • Functional/LaunchActionTest:
    • non-regression of the old format;
    • invalid choice;
    • dates (ATOM);
    • declared file input (.txt refused, , and ; CSV accepted);
    • no free pairs when not allowed;
    • global constraint on a field;
    • pure enum;
    • non-convertible object (500, nothing dispatched);
    • reserved keys;
    • deprecations of a GET (resolved once).
  • Functional/ProcessScheduleCrudTest:
    • invalid key and execution_user refused (422, nothing persisted);
    • valid key persisted;
    • an existing schedule with a key that is now invalid can be enabled/disabled from the list, and saved once its key is fixed.
  • Controller/Admin/Process/LaunchActionTest (new): dispatch() keeps the email of the connected user as execution_user.

Checks

  • PHP 8.5, Symfony 8.1: PHPUnit 588 tests OK, PHPStan level 10 OK, PHP-CS-Fixer OK, Rector OK, twig-cs-fixer OK.
  • The PHP 8.2 + lowest job could not be run locally: to be confirmed by the CI.

Requirements

  • Documentation updates
    • Reference
    • Cookbooks
    • Changelog
    • Upgrade guide
  • Unit tests

Breaking changes

None for a configuration that keeps working with the deprecated options (minor, 3.3). Points to check when upgrading (detailed in UPGRADE.md):

  • Free context pairs of the launch form: renamed from launch[context][<index>] to launch[context][extra][<index>]. context is now a FormType (form theme blocks _launch_context_extra_entry_*).
  • Duplicated default.context keys: merged, the last one wins.
  • Free context keys (launch form and schedules): they must be valid names, and execution_user is reserved. Existing schedules keep running, but must be renamed to be saved from the UI.
  • Invalid notification / logs options: they are no longer reported by the process list, only when used (worker, purge command).

🤖 Generated with Claude Code

…raints, default) through the new form UI option

Add the `form` UI option declaring the `input` field and each context field of the
launch form (type alias or FQCN, form type options with their constraints, default
value), whether free context pairs are allowed, and the constraints of the whole form.
Typed values are converted into process values by the new ContextValueNormalizer.

Deprecate the `entrypoint_type`, `default` and `constraints` UI options (folded into
`form` until 4.0). Reserve the `execution_user` context key, validate the format of the
free context keys (launch form and schedules), resolve only the requested process
option in ProcessConfigurationsManager and the UI options once per launch request.
Require symfony/doctrine-bridge. Add documentation, an upgrade guide and tests.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@njoubert-cleverage njoubert-cleverage left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this work: the feature covers the whole scope of #160, the deprecated options are cleanly folded into form, the deprecation messages name the process, and the docs, upgrade guide and tests (403 → 588) are thorough. CI is green and the CHANGELOG conflict of #161 is solved.

Requesting changes for the points below (details in the inline comments).

Blocking

  1. Scope. The PR bundles three changes that are not part of #160 and deserve their own issue/PR (and changelog entry):
    • execution_user always set to the connected user and reserved (security-related behaviour change, previously a context value could override it);
    • the format rule on the keys of the free context pairs, including the schedules;
    • ProcessConfigurationsManager resolving only the requested option (ui / notification / logs), which changes how invalid notification / logs options are reported.
  2. Key format rule = BC break in a minor. Free keys such as api.token, a/b or a b used to work and are now refused at launch, and existing schedules can no longer be saved from the UI. The root cause is in process-bundle: ContextualOptionResolver builds its regex from the context keys without preg_quote() (that is what the zz)|x|(?:zz / a/b tests show). It should be fixed there; here the naming rule should only apply to the keys declared in form.context, which must be valid form field names anyway.
  3. Form type: FQCN only. FORM_TYPE_ALIASES introduces a second naming scheme for the form types: a closed list of 35 entries to maintain, while any type (including a project-specific one) can already be given by its FQCN. It also requires symfony/doctrine-bridge only for the entity alias. I'd remove it and accept FQCNs only, as the Symfony form builder does and as in the draft of #160: resolveFormType() then only checks class_exists / FormTypeInterface / not a button, and symfony/doctrine-bridge can be dropped from composer.json (docs, changelog and tests to update). This also solves the entity issue: its data is the entity itself, converted through __toString(), i.e. its label rather than its identifier, and its default value cannot be written in YAML.
    The short names of the constraints (Choice, File...) are not concerned: that is the existing ConstraintLoader syntax. A test with a constraint given by its FQCN (e.g. a project-specific constraint) is missing though: it is documented but not covered.

Important

  1. Default values of object types: the docs cover date / dateinterval (input: string), but not enum (a YAML scalar where the field expects an enum case). Either refuse it at resolution or document it.
  2. Late configuration errors: an invalid field option gives a 500 on the launch page, and a non-convertible value gives a 500 after the user filled and submitted the form. At least the latter should be reported as a form error.
  3. Process list: list.html.twig resolves the UI options of every process, so one invalid form configuration breaks the whole page, and every rendering triggers one deprecation per process using the deprecated options.

Minor

  • ProcessConfigurationsManager grows to ~700 lines, mostly the launch form resolution: it could move to a dedicated (internal) resolver.
  • Public API surface: getLaunchFormDefaults() is only used by the tests; buildLaunchFormDefaults(), the ui_options option of LaunchType and ContextValueNormalizer (instantiated with new in the controller) should be @internal.
  • The declared context fields are required: true by default (Symfony default): a checkbox must then be checked (the dry_run example of #160 needs an explicit required: false). A false default for the declared context fields would be safer.
  • The CHANGELOG entry is a single ~2000-character paragraph: it would be easier to read split into bullets.
  • One 3k-line commit makes the review hard: splitting per point 1 would help.

🤖 Generated with Claude Code

// The rule of FormConfigBuilder::isValidName() applied to the declared context keys of the launch form, and
// not only digits (PHP casts such an array key to an integer)
new Regex(
pattern: '/^(?![0-9]+$)[a-zA-Z0-9_][a-zA-Z0-9_\-:]*$/D',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rejects keys that used to work (api.token, a/b, a b...) in the launch form and the schedules: BC break in a minor. The underlying problem is ContextualOptionResolver in process-bundle, which puts the context keys into a regex without preg_quote(); it should be fixed there. Here, the naming rule is only needed for the keys declared in form.context (form field names).

*
* @var array<string, class-string>
*/
private const FORM_TYPE_ALIASES = [

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd drop this alias table and accept FQCNs only for type (as the Symfony form builder and the draft of #160): one naming scheme, nothing to maintain, a project-specific type works the same way, and symfony/doctrine-bridge is no longer needed.

'ulid' => Type\UlidType::class,
'collection' => Type\CollectionType::class,
'repeated' => Type\RepeatedType::class,
'entity' => EntityType::class,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The data of an EntityType is the entity: through ContextValueNormalizer it becomes its __toString() (a label, not the identifier), and its default value cannot be written in YAML. Removing the aliases (see above) removes this one too.

Comment thread composer.json
"symfony/console": "^6.4 || ^7.4 || ^8",
"symfony/dependency-injection": "^6.4 || ^7.4 || ^8",
"symfony/deprecation-contracts": "^2.5 || ^3",
"symfony/doctrine-bridge": "^6.4 || ^7.4 || ^8",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only required for the entity alias: can be removed with FQCN-only types.

*
* @return LaunchFormData
*/
public function getLaunchFormDefaults(string $processCode): array

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only used by the tests: new public API without a caller. Could be removed (or @internal), same question for buildLaunchFormDefaults().

if (null === $recipients) {
return null;
$resolver->setDefaults(['notification' => [], 'logs' => [], 'ui' => []]);
foreach (['notification', 'logs', 'ui'] as $name) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolving only the requested option is a sensible change, but it is not part of #160 and changes how invalid notification / logs options are reported (no longer by the process list): it deserves its own issue/PR.

$context
)
// The connected user always wins over a context value
[ProcessExecuteMessage::EXECUTION_USER_KEY => $this->getUser()?->getEmail()] + $context

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change, but it is a security-related behaviour change unrelated to #160 (a context value could override execution_user before): it deserves its own issue/PR and changelog entry.

return (string) $value;
}

throw new \LogicException(\sprintf('The value of "%s" ("%s") cannot be used as a process context value: use a form type returning a scalar (e.g. "choice" with scalar choices), or make the class implement \Stringable.', $name, get_debug_type($value)));

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is raised after the user filled and submitted the form, and ends as a 500. It would be friendlier as a form error (or detected earlier). Also, this class is instantiated with new in the controller: @internal?

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

Successfully merging this pull request may close these issues.

2 participants