Development to beta#1344
Merged
Merged
Conversation
- updateObjectEntity never called ensureTableForRegisterSchema, so columns added to getMetadataColumns() after a table was created (e.g. _tmlo) were missing when a soft-delete UPDATE ran against older tables. Added the same ensure call that insertObjectEntity already has. - array_diff_key in MagicTableHandler used array_flip() on the result of getExistingTableColumns(), which returns a column-name-keyed map, not a flat list. The flip silently dropped every entry, making every column appear missing and triggering a spurious full sync on every request. Removed the erroneous array_flip(). - Schema properties with format date-time/date were written to MySQL DATETIME columns as raw ISO 8601 strings (e.g. 2024-03-21T10:15:30+00:00) which MySQL rejects. Applied DateTimeNormalizer::formatForDatabase() in prepareObjectDataForTable for these property types, matching the normalisation already in place for system metadata fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ema queries Added $tableColumnsVerifiedCache static property to MagicMapper that records whether a given register+schema table's columns have been verified in the current PHP process. Once verified, ensureTableForRegisterSchema returns immediately without hitting information_schema on subsequent writes. - Fast path in ensureTableForRegisterSchema: if verified flag is set, skip buildTableColumnsFromSchema + getExistingTableColumns entirely - Flag is set to true after a passing sanity check (no missing columns) - Flag is set to true after a successful syncTableForRegisterSchema, covering both the automatic path and the manual frontend sync button (TablesController) - Flag is cleared in invalidateTableCache() alongside existing cache entries - Flag is cleared in clearAllStaticCaches() alongside existing caches - Added setTableColumnsVerified() and isTableColumnsVerified() static accessors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tion DateTime misses DateTimeImmutable — if upstream passes a DateTimeImmutable the value falls through both branches and reaches MySQL unformatted. Using DateTimeInterface covers both concrete types. Fixed in two locations: - System metadata fields (created/updated/expires) around the existing normalisation block - Schema property date-time/date fields added in the previous commit Also adds MagicTableHandlerTest covering the array_flip regression and the $tableColumnsVerifiedCache fast path, as suggested in code review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
fix: resolve HTTP 500 on object delete
Exposes a mapper-like API over ObjectService so external apps (e.g. OpenConnector) can interact with OpenRegister objects through a familiar contract without depending on ObjectService internals. Register and schema context are injected once at construction time. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Wires up the ObjectServiceMapperAdapter so external apps can obtain an instance via ObjectService::getMapper(register, schema). Changes: - ObjectService::getMapper() — returns ObjectServiceMapperAdapter with injected register/schema context; non-numeric string arguments (e.g. 'objectEntity' type hints from OpenConnector) are treated as unconstrained and produce a register/schema-free adapter that searches globally - ObjectService::getValidateHandler() — exposes the internal validate handler - ObjectService::clearCurrents() — resets currentRegister/Schema/Object state - ObjectServiceMapperAdapter::findAllPaginated() — delegates to searchObjectsPaginated() and returns results/total/page/pages Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Also narrows getValidateHandler() return type from mixed to ValidateObject and adds the ValidateObject use statement. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntext patchObject() cast the UUID to int via (int) $objectId, breaking PATCH for UUID-identified objects. updateObject() called saveObject() without the adapter's register/schema context, targeting the wrong table for PUT. Both paths now go through saveObject() directly: - PATCH: fetches existing object via find() (UUID-safe), merges partial data - PUT: passes full data with id set, register and schema injected Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
find() already accepts both IDs and UUIDs. findByUuid was a pure alias with no additional behaviour, kept only for interface compatibility. OpenConnector's EndpointService now calls find() directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add file docblock, constructor docblock, //end markers - Remove spaces around = in default argument values - Replace ! operator with === false - Use named parameter for ValidationException constructor - Fix ?? alignment spacing - Add @return void to clearCurrents() - Remove spaces around = in getMapper() signature Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…-adapter feat: add ObjectServiceMapperAdapter
Collapse PermissionHandler's equality-only evaluateMatchConditions and
MagicRbacHandler's private PHP-side matcher onto the shared ConditionMatcher
service, so schema-level RBAC speaks the same operator and dynamic-variable
grammar as the SQL row-level filter and property-level RBAC. Fixes three
list-vs-find parity bugs surfaced by OpenCatalogi PublicationsController:
1. Schema-level matcher was equality-only and knew only $organisation.
Rules using operators ($lte/$gte/$in/...) or $userId/$now were silently
rejected on find while working on list. Delete evaluateMatchConditions;
inject and delegate to ConditionMatcher (used already by
PropertyRbacHandler). Same pattern applied to MagicRbacHandler's PHP-side
hasPermission; 7 private helpers removed. SQL emitter (applyRbacFilters)
untouched.
2. OperatorEvaluator used raw PHP <=/>=/</> and in_array without null
guards. PHP coerced null to ""/0, so null $lte "<datetime>" returned
true even though SQL NULL <= X correctly filtered the row. Add null
guards to $gt/$gte/$lt/$lte (false if either side null), $in (false
if value null), $nin/$ne (false if value null). $eq: null and $exists
unchanged.
3. $now emitted different string formats in the two evaluators
(ConditionMatcher ISO 8601 'c', MagicRbacHandler Y-m-d H:i:s). For
text/JSON-stored date columns the raw lex comparison diverged at the
T-vs-space position. Align ConditionMatcher to Y-m-d H:i:s (matches
DateTimeNormalizer's input format).
Tests: 102 tests / 148 assertions across OperatorEvaluatorTest (+11 null
cases), PermissionHandlerRbacTest (+14 delegation and real-wired
scenarios including the composite 'publicatiedatum/depublicatiedatum'
pattern), new MagicRbacHandlerTest (14), and unchanged ConditionMatcherTest
(21). composer phpcs/phpmd/psalm/phpstan clean for all changed files.
Integration tests in ObjectHandlersIntegrationTest.php that exercised the
deleted evaluateMatchConditions are removed; their coverage lives in
ConditionMatcherTest and the new delegation tests.
Out of scope (flagged in proposal.md and as follow-ups):
- Schema.php has a 4th duplicate with test coverage but zero production
callers; separate cleanup.
- OpenCatalogi's PublicationsController::show _rbac:false flag, double-
find in ::attachments, FileService::getFiles RBAC integration, and
the 'User Anonymous does not have permission' exception wording are
consumer-side concerns handled elsewhere.
Responds to review feedback on the RBAC unification change:
1. Resolved-relation unwrapping (Wilco, blocker): the deleted
PermissionHandler::evaluateMatchConditions had a fallback that
unwrapped array values with an `id` key to the scalar id. The new
ConditionMatcher path had no equivalent, so rules like
{"match": {"parent": "uuid-123"}} flipped from allow to deny against
schemas with expanded relations. Added unwrapResolvedRelation() to
ConditionMatcher and call it in singleConditionMatches before any
comparison branch. Arrays without an `id` key pass through unchanged.
2. Unknown operator fail-closed (bbrands02, critical): OperatorEvaluator's
default switch arm returned true on unrecognised operators, granting
access on malformed rules while the SQL path correctly denied. Now
returns false and logs a warning, aligning the two paths.
3. $in null guard (Wilco, blocker): the PR claimed `$in/$nin/$ne` return
false for null object values, but operatorIn had been left untouched —
in_array(null, [null, 'x'], true) returns true in PHP while SQL
`NULL IN (NULL, 'x')` returns NULL (filtered out). Added explicit null
guard to operatorIn.
4. Schema::hasPermission and Schema::evaluateMatchConditions now carry
@deprecated annotations pointing to the canonical
PermissionHandler/MagicRbacHandler pair. The methods have no production
callers but extensive test coverage via BasicCrudTest/RbacTest; the
annotations make the §3.7 follow-up visible in-file.
5. PermissionHandler::hasGroupPermission gained an explicit comment on the
`+` array-union precedence in the envelope fold (existing @self wins
over the separately-passed $objectOrganisation — matches pre-unification
behaviour).
6. MagicRbacHandler class docblock tightened: distinguishes local string-rule
dispatch (kept in-class, no operator vocabulary needed) from the
delegated match:-branch evaluation (must live in ConditionMatcher /
OperatorEvaluator going forward).
Tests:
- OperatorEvaluatorTest: flipped testUnknownOperatorReturns* from true
to false; added testInAgainstNullValueWithNullInOperandArrayStillReturnsFalse.
- ConditionMatcherTest: added three tests for resolved-relation unwrapping
(match, mismatch, plain-array non-unwrap).
- PermissionHandlerRbacTest: added two real-wired end-to-end tests
(testResolvedRelationUnwrappingViaRealConditionMatcher and
testUnknownOperatorFailsClosedViaRealConditionMatcher) that exercise
the full handler-to-OperatorEvaluator stack without mocks.
- Refactor: extracted unwrapResolvedRelation helper to keep
singleConditionMatches under PHPMD's NPath/cyclomatic thresholds.
Spec delta: added scenarios for resolved-relation unwrapping, unknown-
operator fail-closed, and a new requirement "Divergences from strict SQL
three-valued logic (documented)" with scenarios for `$eq: null` (escape
hatch) and `$ne: null` (PHP-sensible, SQL-denies-all) so consumers know
where the contract intentionally parts ways.
CHANGELOG: three new bullets covering the three substantive code fixes
(unwrap, unknown-operator, $in null). Aligns the user-facing notes with
what actually ships.
All RBAC tests green (108 tests / 155 assertions). composer phpcs/phpmd
clean for touched files.
…pdate Added concurrency to quality workflow to stop double actions from running
fix: unify RBAC condition matching onto ConditionMatcher (#1336)
rjzondervan
previously approved these changes
Apr 24, 2026
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 147/147 | |||
| npm | ✅ | ✅ 599/599 | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-04-24 10:26 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
requested changes
Apr 24, 2026
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
1 blocker (null dereference on PATCH in new ObjectServiceMapperAdapter) and 3 concerns (timezone divergence in $now, Schema deprecated bypass unaudited in consumer repos, undocumented null-objectData permissive fallback) must be resolved before merge — the RBAC unification itself is solid and the test suite is thorough.
…mArray objectService->find() returns ?ObjectEntity. When the target object does not exist — unknown ID, wrong register/schema scope, or RBAC denial — find() returns null and the unconditional getObject() call produces a fatal TypeError, surfacing as an unhandled 500. Add an explicit null check and throw ValidationException so callers receive a clear, actionable error instead of a cryptic fatal. Using a uniform error message for both "not found" and "not accessible" also prevents ID enumeration through differing error signatures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t-service-mapper-adapter fix: guard against null return from find() in PATCH path of updateFromArray
rjzondervan
previously approved these changes
Apr 24, 2026
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ❌ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 147/147 | |||
| npm | ✅ | ✅ 599/599 | |||
| PHPUnit | ⏭️ | ||||
| Newman | ⏭️ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-04-24 12:04 UTC
Download the full PDF report from the workflow artifacts.
WilcoLouwerse
previously approved these changes
Apr 24, 2026
Contributor
WilcoLouwerse
left a comment
WilcoLouwerse
left a comment
There was a problem hiding this comment.
✅ Blocker resolved (PATCH null guard + ID-enumeration fix in #1346). 3 🟡 concerns and 3 🟢 minor items from my previous review remain open — all pre-existing or low-impact and tracked in the existing threads. Approving.
rjzondervan
dismissed stale reviews from WilcoLouwerse and themself
via
64537f8
WilcoLouwerse
approved these changes
Apr 24, 2026
Contributor
Quality Report — ConductionNL/openregister @
|
| Check | PHP | Vue | Security | License | Tests |
|---|---|---|---|---|---|
| lint | ✅ | ||||
| phpcs | ✅ | ||||
| phpmd | ✅ | ||||
| psalm | ✅ | ||||
| phpstan | ✅ | ||||
| phpmetrics | ✅ | ||||
| eslint | ✅ | ||||
| stylelint | ✅ | ||||
| composer | ✅ | ✅ 147/147 | |||
| npm | ✅ | ✅ 599/599 | |||
| PHPUnit | ❌ | ||||
| Newman | ❌ | ||||
| Playwright | ⏭️ |
Quality workflow — 2026-04-24 12:20 UTC
Download the full PDF report from the workflow artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.