Skip to content

Add DatabaseObjectBuilder with TagBuilder implementation#6689

Draft
BurntimeX wants to merge 18 commits into
6.3from
63-dbo-builder
Draft

Add DatabaseObjectBuilder with TagBuilder implementation#6689
BurntimeX wants to merge 18 commits into
6.3from
63-dbo-builder

Conversation

@BurntimeX

Copy link
Copy Markdown
Member

Introduces an abstract builder for creating, updating and deleting database objects with a fluent setter API, batched transactional deletes and an INSERT IGNORE-style helper. TagBuilder is the first concrete implementation.

Introduces an abstract builder for creating, updating and deleting database objects with a fluent setter API, batched transactional deletes and an `INSERT IGNORE`-style helper. `TagBuilder` is the first concrete implementation.
Comment thread wcfsetup/install/files/lib/data/tag/TagBuilder.class.php Outdated
Comment thread wcfsetup/install/files/lib/data/DatabaseObjectBuilder.class.php Outdated
Comment thread wcfsetup/install/files/lib/data/DatabaseObjectBuilder.class.php Outdated
Comment thread wcfsetup/install/files/lib/data/DatabaseObjectBuilder.class.php Outdated
BurntimeX added 16 commits June 25, 2026 19:51
Replace the `TagAction`-based persistence in `TagAddForm`/`TagEditForm` with the new `DatabaseObjectBuilder` flow.
Introduce a counterpart to `saveValueCallback()` that loads a field's value from an `IStorableObject` when an edit form is populated. When set, the callback takes precedence over the default property-based loading in `updatedObject()`, allowing values that must be derived from a related object or an additional query.
Provides a static helper to atomically increment or decrement counter columns for a given object, mirroring the behavior of the legacy `DatabaseObjectEditor::updateCounters()`.
The method was based on `DatabaseObjectEditor::fastCreate()` which is used very rarely and therefore is not needed in the new API.

The change simplifies the code and allows the DBO to be passed directly as a parameter to `afterCreate()` and `afterUpdate()`
@BurntimeX BurntimeX requested a review from dtdesign July 3, 2026 19:43
Comment thread wcfsetup/install/files/lib/data/tag/TagBuilder.class.php
Comment thread wcfsetup/install/files/lib/system/form/builder/field/IFormField.class.php Outdated
@dtdesign

dtdesign commented Jul 5, 2026

Copy link
Copy Markdown
Member

Pattern for Builders

We need to formalize the pattern at some point, for now I think it’s reasonable to go with:

  • Methods should be named set*(): static and use camel case, for example, setTags() or setIsDeleted().
  • Whenever possible, methods should be designed to make invalid states unrepresentable. Do not offer a separate setUserID() and setUsername() method, instead offer setUser(User $user) and setGuest(string $username) which internally sets both properties (userID and username).
  • Parameters must be as strictly typed as reasonably (!) possible. Instead of asking for ?int $languageID, require a ?Language $language to be provided instead.
  • Any extra state must be tracked in private properties that are both typed and not initialized. A definition like private ?Foo $foo; allows us to distinguish between ”do not consider” and null. Use this for all types in order to have a common pattern.
  • Do not perform any validation beyond checking for the minimum required properties to formally create an object. Use afterValidateCreate() to run checks for extra state only that must be present.
  • Any methods that runs in afterCreate() or afterUpdate() should be named save*() in order to distinguish them from the set*() methods.

Things not yet decided

Support for Implicit Default Values

Not every value can be mapped through MySQL’s default values, for example, we cannot set default values for *TEXT columns nor are we able to utilize UNIX_TIMESTAMP() for timestamps since we require them to be consistent within a request. A common use case is to implicitly set the time column to \TIME_NOW.

However, this can be extended to other kind of columns, such as auto-filling the user. DBOActions usually set it to whatever is present in WCF::getUser() which is dangerous when executed in a programmatic fashion. It is also debatable wether or not this actually saves a lot of work, typically these builders are part of a form and the effort to just manually invoke ->setTime(\TIME_NOW) is pretty much negligible.

We could add this as an indirect feature to the Create* commands but this has the issue that the contents of the builder is not visible on the outside.

Asymmetric Visibility on Builder Properties (PHP 8.4)

PHP 8.4 has the asymmetric visibility that would allow us to define the properties as private(set). This would also make it possible to make the changes performed in forUpdate() transparent to event listeners by providing the builder. Listeners can then inspect the changes and decide if they want to take an action.

The major drawback of this is that Ubuntu 22.04 LTS ships with PHP 8.3 (Debian Trixie offers PHP 8.4) natively, which would mean that this would be a bit of a steep increase in system requirements. That said, PHP 8.3 is already out of support, with security support ending Jan 1, 2028. It is already a pretty old version of PHP.

See https://www.php.net/releases/8.4/en.php#asymmetric_visibility to learn more.

@dtdesign

dtdesign commented Jul 6, 2026

Copy link
Copy Markdown
Member

Both the Create* and Update* command expect a builder instance but they do not verify if this is actually for an update. They will simply request the object and later on interact with it, expecting it to be not null, eventually causing an error when accessing properties on null.

Maybe add a new helper method that fails loud when accessing the object on a create instance? Or just turn the getObject() method into something that throws an exception? The intention is to avoid having the same boiler-plate code in each Update* command just to check it and throw.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants