MokoConsulting/MokoSuiteOpenGraph
1
0
Fork 0
Code Issues 4 Pull Requests 1 Releases 2 Wiki Activity

Minor correctness bugs: loadArticle null-cache, zero-date OG timestamps, batch JS premature halt #106

New Issue
Closed
opened 2026-06-29 14:20:25 +00:00 by jmiller · 3 comments
jmiller commented 2026-06-29 14:20:25 +00:00
Owner
Copy Link
Copy Source

Three small, independent correctness bugs

  1. loadArticle() per-request cache never caches misses — plg_system_mokoog/src/Extension/MokoOG.php:669-692. A miss stores null in $cache[$id], but the guard is isset($cache[$id]) (false for null), so a missing/unpublished article is re-queried on every call within the request. Use array_key_exists() (or a sentinel) so negative lookups are cached.

  2. Zero-dates emitted as invalid OG timestamps — getArticleDate() (~:220-221). publish_up/modified of 0000-00-00 00:00:00 are output verbatim as article:published_time / article:modified_time, producing invalid metadata. Skip/normalise zero dates.

  3. Batch JS stops on a fully-skipped chunk — tmpl/tags/default.php:230. The loop continues only if (resp.data.created > 0 && processed < total). If an entire chunk inserts nothing (e.g. all unique-key conflicts → created = 0) while eligible articles remain, progress halts before 100%. Continue based on processed < total, not created > 0.

Each is small; bundled for convenience but can be split.

## Three small, independent correctness bugs 1. **`loadArticle()` per-request cache never caches misses** — `plg_system_mokoog/src/Extension/MokoOG.php:669-692`. A miss stores `null` in `$cache[$id]`, but the guard is `isset($cache[$id])` (false for `null`), so a missing/unpublished article is **re-queried on every call** within the request. Use `array_key_exists()` (or a sentinel) so negative lookups are cached. 2. **Zero-dates emitted as invalid OG timestamps** — `getArticleDate()` (~`:220-221`). `publish_up`/`modified` of `0000-00-00 00:00:00` are output verbatim as `article:published_time` / `article:modified_time`, producing invalid metadata. Skip/normalise zero dates. 3. **Batch JS stops on a fully-skipped chunk** — `tmpl/tags/default.php:230`. The loop continues only `if (resp.data.created > 0 && processed < total)`. If an entire chunk inserts nothing (e.g. all unique-key conflicts → `created = 0`) while eligible articles remain, progress halts before 100%. Continue based on `processed < total`, not `created > 0`. Each is small; bundled for convenience but can be split.
jmiller added the bug label 2026-06-29 14:20:25 +00:00
jmiller commented 2026-06-29 14:20:27 +00:00
Author
Owner
Copy Link
Copy Source

Branch created: feature/106-minor-correctness-bugs-loadarticle-null-

git fetch origin
git checkout feature/106-minor-correctness-bugs-loadarticle-null-
Branch created: [`feature/106-minor-correctness-bugs-loadarticle-null-`](https://git.mokoconsulting.tech/MokoConsulting/MokoSuiteOpenGraph/src/branch/feature/106-minor-correctness-bugs-loadarticle-null-) ```bash git fetch origin git checkout feature/106-minor-correctness-bugs-loadarticle-null- ```
jmiller commented 2026-06-29 14:54:57 +00:00
Author
Owner
Copy Link
Copy Source

Partially fixed in PR #109 (merged to dev):

  • ✅ #1 loadArticle() now caches null misses via array_key_exists() — no more per-call re-query.
  • ✅ #2 getArticleDate() skips 0000-00-00 dates (no more invalid article:published_time/modified_time).

Still open — #3 (batch JS premature halt): investigating it revealed the guard is load-bearing. BatchController::process() always queries from offset 0 and relies on the IS NULL join to exclude done rows; a row that fails to insert is never excluded, so it's re-fetched every chunk. The current created > 0 stop condition is what prevents an infinite loop on a persistently-failing row. A correct fix requires cursor-based pagination (e.g. WHERE c.id > :lastId) in BatchController so failed rows are skipped — leaving this open for that redesign.

**Partially fixed in PR #109** (merged to `dev`): - ✅ #1 `loadArticle()` now caches `null` misses via `array_key_exists()` — no more per-call re-query. - ✅ #2 `getArticleDate()` skips `0000-00-00` dates (no more invalid `article:published_time`/`modified_time`). **Still open — #3 (batch JS premature halt):** investigating it revealed the guard is load-bearing. `BatchController::process()` always queries from offset 0 and relies on the `IS NULL` join to exclude done rows; a row that **fails** to insert is never excluded, so it's re-fetched every chunk. The current `created > 0` stop condition is what prevents an **infinite loop** on a persistently-failing row. A correct fix requires cursor-based pagination (e.g. `WHERE c.id > :lastId`) in `BatchController` so failed rows are skipped — leaving this open for that redesign.
jmiller commented 2026-06-29 17:18:27 +00:00
Author
Owner
Copy Link
Copy Source

All three items now resolved. The batch-JS halt (#3) is fixed in PR #116 (merged to dev): BatchController::process() paginates by id cursor (WHERE c.id > :lastId ORDER BY c.id), so a row that fails to insert falls behind the cursor and is never re-fetched — the batch is provably terminating and reaches 100% even with persistent failures. The JS drives the cursor and stops when a chunk examines 0 rows. (Items #1 loadArticle null-cache and #2 zero-date were fixed earlier in PR #109.) Closing.

All three items now resolved. The batch-JS halt (#3) is fixed in PR #116 (merged to `dev`): `BatchController::process()` paginates by **id cursor** (`WHERE c.id > :lastId ORDER BY c.id`), so a row that fails to insert falls behind the cursor and is never re-fetched — the batch is provably terminating and reaches 100% even with persistent failures. The JS drives the cursor and stops when a chunk examines 0 rows. (Items #1 loadArticle null-cache and #2 zero-date were fixed earlier in PR #109.) Closing.
Sign in to join this conversation.
Labels
Clear labels
bug
Something is broken
 documentation
Documentation improvements
 enhancement
production-readiness
Required before v1.0 release
 security
Security vulnerability or hardening
 testing
Test coverage and QA
No labels bug
Priority Medium
Type Feature
Milestone
No items
No Milestone
Assignees
Clear assignees
jmiller (Jonathan Miller)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: MokoConsulting/MokoSuiteOpenGraph#106
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?