fix: TypeError on post, category, tag, author save with empty URL key - #17
Merged
Merged
Conversation
Saving a post, category, tag or author with the url_key (Author: slug) field left blank died with a TypeError. All four admin Save controllers hydrate scalars in one loop that maps '' and null to $setter(null), and url_key was in that list, but Model\Post::setUrlKey(string) and its three siblings are non-nullable, so the loop threw before the generate-from-title fallback below it could ever run. Take url_key/slug out of that loop and route every write through a new UrlKeyResolver, which resolves in order: submitted value, then the value already stored, then generated from the title. Putting the stored value ahead of the title means blanking the field on an edit keeps the current URL instead of silently moving the page. When nothing is usable (no title, or a reserved or unsluggable one) it throws LocalizedException so the existing catch shows a form message rather than a 500. Submitted slugs are now normalized through the same rules as generated ones, so a hand-typed "My Slug!" becomes my-slug instead of being stored verbatim and breaking the URL. Normalization moved out of UrlKeyGenerator into SlugNormalizer so the resolver can reuse it without also triggering generate()'s collision suffixing: an explicitly typed slug that clashes must surface as a validation error, not become my-slug-2 behind the editor's back. Same defect, other entry points, fixed the same way: - GraphQL create/update resolvers for all four entities cast $input['url_key'] to (string), turning an omitted optional field into '', which then failed validate() with a confusing "URL key '' is already in use". - The inline-edit grids expose url_key/slug as editable columns and write them through setData(), bypassing the typed setter: blanking a cell stored an empty slug and broke that entity's URL. UrlKeyGenerator gains a constructor argument, so this needs setup:di:compile on any install with compiled DI.
Sental
self-requested a review
July 27, 2026 16:17
Contributor
|
Thanks for the PR, appreciate the time you put into it — clean fix and good tests. A few things to possibly consider: Where url_key integrity lives. The resolver holds the logic in one place, but all 20 call sites still have to remember to call it. The model itself never guarantees a Post has a valid url_key. So any future save path that skips the resolver — a new controller, a REST endpoint, an import, another module calling Consider enforcing it at save time instead: a Other minor things:
|
Removed UrlKeyResolver and updated the handling of `url_key`/`slug` across admin save controllers, GraphQL resolvers, and inline-edit grids. Writes now set the field directly if present, without relying on resolver logic. Updated unit tests to validate the new behavior.
Saving a post, category or tag produced no url_rewrite row, so the pretty URL 404ed and getByUrlKey() stopped finding the entity. Two defects: the store multiselect had no "All Store Views" option, so an empty selection was the only reachable state, and an empty store list wiped the store pivot (delete-then-insert sync) after which the plugins returned early without writing anything. - UrlRewriteBuilder treats an empty store list like store 0: rewrites for every store view, so the plugins no longer bail silently - the plugins drop the empty-store gate - Save controllers parse store ids with parseStoreIds(), which keeps 0 and collapses it to the single "all stores" marker - new Ui\Component\Form\Stores\Options adds the missing "All Store Views" entry to the three forms - Category and Tag form data providers hydrate store_ids, which they never round-tripped, so editing no longer resets the assignment Needs setup:di:compile (data provider constructors changed).
Every post, category and tag saved before the rewrite fix has zero rewrite rows and an emptied store pivot, so the data needs a backfill. mageos:blog:url-rewrite:regenerate [--entity=post|category|tag|author|all] [--dry-run] gives entities with no pivot row a store_id 0 assignment, then rebuilds their rewrites through UrlPersistInterface::replace(). Per-entity failures are reported and the run continues with a non-zero exit code.
2 tasks
The refactor changed UrlKeyResolver::resolve() to take a SlugEntity and a SlugCandidates object, but the integration test still passed the old string arguments, so all six cases died with a TypeError on 2.4.9 CI.
Contributor
|
Thanks for the type error fix but this doesn't solve a real world "I can't get to my post" issue. The full fix for Issue #16 & #18 may solve that. I'm not willing to approve a PR that solves a technical problem but leaves a real problem on the table. Please merge the fixes into one PR and then I will consider it. |
Contributor
Author
|
Hi @Sental , I've merged, let me know. |
The builder takes the generated UrlRewriteFactory, which does not exist in a standalone module checkout, so mocking it broke the mutation testing and PHPStan jobs. PHPUnit 13 no longer allows mocking unknown types either. Store expansion is already asserted end to end by the url rewrite plugin integration tests, and infection.json5 excludes Model/Url for the same reason: framework coupled classes belong in Test/Integration here.
Replaces the unit test that had to mock the generated UrlRewriteFactory. Entities are built in memory, so no repository save fires the rewrite plugins and the builder's own output is what gets asserted. Covers explicit store ids, duplicates, store 0 and an empty list expanding to every store, the category, tag and author paths, and redirect_type plus is_autogenerated on the generated rows. The multi store cases use the core store fixture so they do not pass trivially on a single store install.
Sental
approved these changes
Aug 12, 2026
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
3 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.
Summary
Saving a post, category, tag or author with the url_key (Author: slug) field left blank died with a TypeError. All four admin Save controllers hydrate scalars in one loop that maps '' and null to $setter(null), and url_key was in that list, but Model\Post::setUrlKey(string) and its three siblings are non-nullable, so the loop threw before the generate-from-title fallback below it could ever run.
Motivation
Saving a post, category, tag or author with an empty URL Key / Slug no longer throws a TypeError (#16).
Closes #16
How to test
main.CRITICAL: TypeError: MageOS\Blog\Model\Post::setUrlKey(): Argument ($urlKey) must be of type string, null given, called in Controller/Adminhtml/Post/Save.php on line 94 and defined in Model/Post.php:46
Same process for category, tag and author.
Checklist
main.feat:,fix:,refactor:,test:,chore:,docs:).vendor/bin/phpunit --testsuite unitpasses locally.vendor/bin/phpstan analyse --memory-limit=1Gpasses locally.vendor/bin/phpcs --standard=phpcs.xml.distpasses locally.vendor/bin/php-cs-fixer fix --dry-run --diff --allow-risky=yesshows no changes needed.declare(strict_types=1);.CHANGELOG.mdentry under## [Unreleased].N/A, release-please handles it
CONTRIBUTING.md).Screenshots / GraphQL samples (if UI or API change)