Uh oh!
There was an error while loading. Please reload this page.
Add sitemap_sources/0 module callback for source auto-registration - #603
Conversation
…on_spaces DBs that ran V91 before the locations feature was added to it never got the phoenix_kit_locations table. V122 adds phoenix_kit_location_spaces with a FK to it, so the migration failed with undefined_table on those installs. Create the parent table idempotently (create_if_not_exists, mirroring V91) before the child; a no-op where V91 already created it.
…e location_spaces" This reverts commit ab9b1fa.
External PhoenixKit modules can now contribute sitemap sources via an optional sitemap_sources/0 callback on PhoenixKit.Module. The sitemap Generator merges these (deduplicated) with its built-in sources, so a module's content is included with no host-app config — matching the existing route/css/js auto-discovery pattern. - PhoenixKit.Module: new optional sitemap_sources/0 callback (default []). - ModuleRegistry.all_sitemap_sources/0 aggregates them across modules. - Generator.get_sources/0 appends module sources to the base list (host config or defaults), deduped and order-preserving.
timujinne
left a comment
There was a problem hiding this comment.
Summary
This PR adds an optional sitemap_sources/0 callback to PhoenixKit.Module, a ModuleRegistry.all_sitemap_sources/0 aggregator, and wires module-contributed sources into Sitemap.Generator.get_sources/0 (append + dedup). The behaviour wiring is clean and complete — the callback is added consistently to @callback, @optional_callbacks, the __using__ default (def sitemap_sources, do: []), and defoverridable, matching the existing css_sources/js_sources pattern exactly. Failure isolation is handled at two layers (safe_call per-module in the registry, Source.safe_collect per-source at collect time). Overall this is a well-targeted, low-risk change. The points below are mostly about documenting/scoping the backward-compat shift and one enable-state edge case.
Findings
BUG - MEDIUM
lib/phoenix_kit/module_registry.ex:188-193(all_sitemap_sources/0) iteratesall_modules(), i.e. all registered modules regardless of enabled state, unlike the natural expectation that a disabled module contributes nothing. The mitigation is indirect:Source.safe_collect/2(generator.ex:444,source.ex:110) gates on the source module'senabled?/0, which is a separate concept from the PhoenixKit module'senabled?/0. So a module that an operator has disabled in the registry can still have its sitemap source emit entries if that source's ownenabled?/0returnstrue. If the intent is "disabled module → no sitemap contribution," switch toenabled_modules()here (consistent with how a feature toggle should behave), or document explicitly that source inclusion is governed solely by the Source'senabled?/0, not the module toggle.
IMPROVEMENT - HIGH
lib/modules/sitemap/generator.ex:425-435— the backward-compat semantics shift is real and silent: previouslyconfig :phoenix_kit, sitemap: [sources: [...]]gave the host an authoritative, exhaustive source list. Now module sources are unconditionally appended with no opt-out. A host that intentionally narrowed:sources(e.g. omittedShopor pruned to a minimal set) can no longer prevent a module from re-introducing content viasitemap_sources/0. Consider one of: (a) a config escape hatch likesitemap: [include_module_sources: false], and/or (b) a one-line note in theget_sources/0@doc falsecomment and themodule.excallback docs stating that an explicit:sourcesconfig is now a base list that module sources extend rather than replace. Even without the escape hatch, the doc note prevents a surprising regression for existing hosts.
IMPROVEMENT - MEDIUM
lib/modules/sitemap/generator.ex:437-441—module_sitemap_sources/0wrapsall_sitemap_sources/0inrescue _ -> [], butall_sitemap_sources/0already routes every per-module call throughsafe_call/3, which rescues and logs individually. The only thing the outer rescue catches is a registry-level failure (e.g.:persistent_termnot yet populated), and it swallows it silently with a barerescue _. If you keep it, narrow it and log (rescue e -> Logger.warning(...) ; []) so a genuinely broken registry isn't invisible; otherwise it can be dropped since the inner layer already isolates the realistic failure mode.
NITPICK
lib/phoenix_kit/module_registry.ex:192andlib/modules/sitemap/generator.ex:434both callEnum.uniq()on the source list, and the final entries are deduped again downstream byuniq_by(& &1.loc)(generator.ex:449). TheEnum.uniq()insideall_sitemap_sources/0is redundant with the one inget_sources/0after thebase_sources ++concat — a single dedup at theget_sources/0level would suffice. Harmless, just slightly belt-and-suspenders.lib/phoenix_kit/module.ex— no test accompanies the new callback. A small unit test asserting (a) the__using__default returns[], (b)all_sitemap_sources/0flattens + dedups across two fixture modules, and (c) a raisingsitemap_sources/0is isolated to[]would lock in the failure-isolation contract that is the main value of this PR.
Verdict
Looks good to merge after addressing the IMPROVEMENT - HIGH backward-compat documentation (and deciding on the enabled-vs-all-modules question in BUG - MEDIUM). No correctness blockers; the wiring itself is correct and idiomatic.
Addresses review feedback on the sitemap_sources/0 auto-registration: - get_sources/0: note that a host :sources config is now a BASE list extended by module sources (not a full replace), and that each source's own enabled?/0 gates emission. - ModuleRegistry.all_sitemap_sources/0: clarify it iterates all registered modules, with the source's enabled?/0 as the generation-time gate.
… mode PR #603's `all_sitemap_sources/0` iterated `all_modules/0`, relying on each source's own `enabled?/0` to gate emission at generation time. That holds in index mode, but flat mode (`do_generate_flat`) passes `force: true`, which `Source.safe_collect/2` honors by bypassing `enabled?/0` — so a disabled module that registered a `sitemap_sources/0` entry would still publish its URLs, contradicting the PR's documented "disabled module emits nothing" guarantee. Gate at the module level instead: iterate `enabled_modules/0` so a disabled module contributes no source at all, in both modes. A source's own `enabled?/0` remains a secondary gate in index mode. The intentional flat-mode force-collect (commit c853379) is left intact for the sources that are in the list. Also add post-merge review docs for #603 and #604. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Releases the unpublished work on main since 1.7.164: - #603 sitemap_sources/0 auto-registration (+ enabled-module gating fix) - #604 V137 email event dedup indexes + aws_message_id backfill - #605 notifications graceful handling + user/file comment-resource links (+ cached default-link lookup) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Summary
External PhoenixKit modules could not contribute entries to the generated sitemap:
Generator.get_sources/0only read a hardcodeddefault_sources/0list (router discovery, static, publishing, posts, shop) or a hostconfig :phoenix_kit, :sitemap, sources:override. There was no auto-discovery, so a module shipping its ownSitemap.Sources.Source(e.g. Entities) was never invoked unless every host hand-wired it into config.This adds a zero-config registration path mirroring the existing route / CSS / JS auto-discovery.
Changes
PhoenixKit.Module: new optionalsitemap_sources/0callback (default[]), added to@optional_callbacks, the__using__defaults, anddefoverridable.PhoenixKit.ModuleRegistry.all_sitemap_sources/0: aggregates the callback across all registered modules (deduped), using the existingsafe_call/3.PhoenixKit.Modules.Sitemap.Generator.get_sources/0: appends module-contributed sources to the base list (host config or defaults) and dedups, order-preserving. Backward compatible — a host that sets:sourceskeeps it as the base; module sources are appended.Test Plan
mix compilecleanmix credo --strict— no issuesmix dialyzer— passedGenerator.get_sources/0now includes a module-contributed source and its URLs appear in the generated sitemap.