Split the framework into declare, compile and run layers - #8
Open
mikield wants to merge 11 commits into
Open
Conversation
A #[Command] whose __invoke takes no options produced zero handlers: the handler map was seeded with an empty array and only filled from inside the options loop, so with no options the loop never ran and dot() dropped the empty branch. The command registered with Discord and then silently never fired. Handlers are now derived from whether the command has subcommands at all, rather than by looping over options and letting the last iteration win. That also stops a command declaring both options and subcommands from clobbering its subcommand handlers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
mapValue() awaited rest->user->get() in a standalone if-block and then awaited the identical call again in the match arm below it, discarding the first result. Every user-typed option cost two REST round trips. No regression test yet: mapValue reaches Discord through get(Tempcord::class), and Tempcord's constructor needs a live gateway, so it cannot be exercised without a container. Covered once the dependency is injected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two defects in the autocomplete responder: The 25-choice cap never applied. array_map iterated $value rather than the capped $choices, so the array_slice result was computed and thrown away and every suggestion the Autocomplete returned went to Discord, which rejects responses over 25. For the same reason a non-array return — a bare scalar standing for a single suggestion — reached array_keys() and TypeError'd, and array_is_list was checked against the capped array while the labels came from the uncapped one. resolveFocusedAndParam returns null whenever nothing is focused or the focused option is not declared on the command, which Discord does send while the user is still typing. Destructuring that null read properties off null, emitting warnings and dropping the response. Choice building moves to CommandsRegistry::toChoices so it can be tested directly; the null is now guarded explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
resolveFocusedAndParam compared $option->type->value against a bare [1, 2], the one place in the codebase that reaches past ApplicationCommandOptionType instead of using it. Comparing the enum cases directly says what it means and survives any renumbering. Adds the nested coverage this path was missing: drilling through a group and a subcommand to the focused option, and a group the definition does not declare. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The attributes were doing five jobs at once: declaring a command, walking the
class to find its parts, building the Discord payload, reaching into the
container, and invoking the handler. Everything awkward about the codebase
followed from that.
Because logic lived on objects PHP constructs for you, the reflector had to be
assigned from outside after the fact, so every property was a landmine until
discovery had run. Typed properties could not be used for the declared values,
so HasAttributes stood in as an untyped bag. The scanning logic could not be
called on its own, so Command::options built throwaway "fake" Subcommand and
SubcommandGroup instances to borrow it. And since the getters re-ran reflection
on every read, the tree had no stable identity at all: reading ->options twice
returned different objects, and a stateful Autocomplete was reconstructed on
every interaction, silently discarding anything it had cached.
The lifecycle those getters implied is now explicit:
Attributes inert readonly declarations, no reflection, no logic
Compiler ClassReflector + attributes -> immutable definition tree, once
Definitions CommandDefinition and friends, including handlers flattened to
the dotted paths Discord reports, each carrying the option path
it reads from
Discord CommandBuilderFactory, the one place Fenrir's builders are used
Runtime CommandDispatcher, ArgumentResolver, OptionValueResolver,
AutocompleteResponder, ChoiceFactory
Two dispatch defects fall out of moving argument binding into ArgumentResolver:
An option renamed with #[Option(name: ...)] was keyed by the name Discord knows
it under while parameters were matched by their PHP name, so the two never met
and the handler failed with "Missing required parameter". Arguments are now
keyed by the parameter they are destined for.
An optional option the user left out was supplied as null rather than left
absent, so the parameter default never applied and any non-nullable parameter
raised a TypeError. Absent options are now skipped.
Other consequences worth calling out:
Compiling once at discovery means an unsupported option type now fails on boot
rather than on the first interaction that happens to reach it.
Because handlers know the path each of their options sits at, autocomplete
resolution is a direct lookup and the recursive walk back down the subcommand
tree is gone.
Dependencies are injected rather than fetched: OptionValueResolver takes a
Discord, the registries and dispatcher take a Container, and Tempcord takes its
registries. Nothing in the command path calls get() any more, which is what
makes the runtime testable without a gateway. A command that throws is now
logged and contained rather than escaping into the gateway loop.
Registries report Outcomes instead of printing, and BootCommand renders them,
so the domain no longer owns presentation and tests assert on results rather
than mocking Console.
The user-facing API is unchanged: bot authors only ever write attributes, and
those keep the same names and arguments. AllCommandExtension and
InteractionCallbackBuilder move into Tempcord\Discord.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sions Two fixes that come from the same place. Registration used to POST each command individually. That costs a request per command, and Discord's create endpoint only ever adds: a command deleted from the code stayed registered forever and users kept seeing it. Registration now groups commands into the sets Discord replaces atomically — the global set, and one set per guild — and sends each as a single PUT. Anything absent from the set is removed, which is what makes a deleted command actually disappear. #[Command(permissions: [])] was collected all the way into CommandDefinition and then never read, so setting it did nothing. It is now typed as Fenrir's Permission enum rather than a list of strings, and reaches the payload. Two things are worth explaining rather than leaving to be discovered: The permission bit field is written onto the payload directly instead of going through CommandBuilder::setDefaultMemberPermissions, which sends the binary representation of the bit field where Discord expects the decimal one. Asking for ADMINISTRATOR that way grants five permissions nobody asked for. Fixed upstream in dc-Ragnarok/Fenrir#134; the direct write is correct either way, so it does not need reverting when that lands. Fenrir has no bulk overwrite method yet and keeps its HTTP client private on both Discord and Rest, so CommandRegistrar builds one for itself from the token already in TempcordConfig. Registration runs once, before the gateway opens, so the two clients never compete for a rate limit bucket. The method is added in dc-Ragnarok/Fenrir#133 and this can move over to it once released.
Fenrir's CommandOptionBuilder has always supported choices, numeric bounds,
string lengths and channel type filters. None of them were reachable through
#[Option], so a bot author had to validate inside the handler what Discord
would happily have enforced before the command ever fired.
#[Option(description: 'Size', choices: ['Small' => 's', 'Large' => 'l'])]
#[Option(description: 'Count', minValue: 1, maxValue: 10)]
#[Option(description: 'Note', minLength: 2, maxLength: 32)]
#[Option(description: 'Where', channelTypes: [ChannelType::GUILD_TEXT])]
Choices accept either shape. A map uses its keys as the labels users see; a
list has no labels of its own, so each value stands in as its own. That mirrors
how ArrayAutocomplete already reads its items, so the two behave the same way.
Constraints are omitted from the payload entirely when not declared, rather
than sent empty.
Closes #2. The issue proposed adding descriptionLocalizations to the option attribute and wondered whether there was a better way to manage the translations. There is, and Tempest already had it: tempest/intl ships a translation catalog, so command translations can live in the same files as the rest of an application's rather than inline in PHP attributes, where translators cannot reach them and where they cannot be anything but constant expressions. A command declares one key and everything beneath it follows from position: #[Command(description: 'Music controls', translationKey: 'commands.music')] #[SubcommandGroup(name: 'playlist', description: 'Playlist controls')] final class MusicCommand { #[Subcommand(name: 'play', description: 'Play a track')] public function play( #[Option(description: 'Track title')] string $title, ): void {} } commands.music.description commands.music.playlist.description commands.music.playlist.play.description commands.music.playlist.play.title.description Names are localized the same way, on the same four levels, since Discord localizes both and solving one without the other would mean designing this twice. Only locales the catalog actually has are sent. A translator returns the key itself when a message is missing, so asking it directly would have shown "commands.music.description" to German users; the catalog is asked whether it has the message instead, and absent locales are left out so Discord falls back to the declared text. A command with no translationKey asks for nothing at all. tempest/intl is not made a dependency. It needs ext-intl and ext-dom, which is a lot to require of a bot that does not translate anything, so it is declared as a suggestion. Only CatalogLocalizations touches it, and an initializer returns NullLocalizations when the package is absent, keeping the class from being autoloaded at all. DiscordLocale carries the mapping between the two locale spellings, since Discord writes en-GB where Tempest writes en_GB. Three have no direct equivalent and are mapped deliberately: Tempest has no Latin America wide Spanish, so es-419 reads the unqualified Spanish catalog while es-ES reads Spain's, and Discord's two Chinese regions map onto Tempest's simplified and traditional scripts. A test pins all thirty-four against Tempest's enum so the mapping cannot rot silently.
The documentation in docs/ described a framework that does not exist. There was a 638 line page on a middleware layer the codebase has never had, an OptionType class that was never written, and #[Option] shown on methods rather than parameters. It came from the "AI Generated documents" commit and was never true. Publishing it on a website would mislead everyone who read it. So it is replaced, and split by what can be derived and what cannot. The reference is generated by reflecting over the public surface: the attributes, the Autocomplete interface and its array implementation, the config object, and the DiscordLocale enum. Parameter names, types, defaults and descriptions come from the constructor and its docblock, and which declarations an attribute may be written on comes from its #[Attribute] flags rather than from prose. It cannot describe something that is not there. The guides are hand written, because narrative cannot be derived — but they carry no code of their own. Every example is transcluded from a file under tests/Fixtures, which the suite already compiles and exercises, so an example that stops being true breaks the build. Including a file that does not exist is an error rather than a blank. Two tests hold the line: one regenerates everything and fails if what is committed no longer matches the source, and one checks every included path exists. That is the check the old documentation never had. Output is markdown plus an index.json carrying the same information structurally, so the website can build navigation and search without parsing prose, while the markdown stays readable on GitHub and reviewable in a diff. The generator lives in tools/ under autoload-dev, so it is not shipped to bots that install the framework. Run it with `composer docs`. phpstan now covers it. Fixing it turned up two things worth noting: a docblock type containing a space, as in array<string, int>, defeated the parameter parser and silently dropped the description, and the README's own example passed a required: true argument that #[Option] has never accepted.
Closes #1. The issue asks for discovery so that plugins can provide their own commands and events. Most of that already works, and not because of anything here: Tempest discovers any installed package that depends on a tempest/* package, and Tempcord's discovery never looks at where a class came from. A package shipping #[Command] or #[Event] classes is already picked up by the bot that installs it. A package that depends on nothing from Tempest can opt in with extra.tempest.can-discover instead. What discovery cannot do is give a package a moment to run — to register a Fenrir extension, start a timer, or open a connection of its own. That is the part that was missing, and it is why tempcord/tasks is written against a Tempcord\Plugins\Plugin interface that has never existed. So: a Plugin interface with a single boot method, a discovery that finds implementations, and a registry that boots them as part of listening, after commands and events are bound and before the gateway opens. Plugins are built by the container, so they may take dependencies. Implementing the interface is the whole registration. Two decisions worth stating. A plugin that throws while booting is logged and reported but does not stop the bot, matching how a command or event listener that throws is contained. And plugins are keyed by class, since discovery can reach the same class through more than one location and booting twice would register a plugin's extensions twice. The interface deliberately has no register hook. Tempest's container already handles service registration through discovery and initializers, and the one plugin written against the imagined API had an empty register method.
Everything here was found by installing the framework into the starter application and running it. The test suite passed throughout, because unit tests hand collaborators in directly and never exercise the container assembling the graph for real. Registries no longer depend on anything. Discovery builds them while the container is still being assembled, before the initializers that provide services like the logger have themselves been discovered — so a registry reaching for a logger meant the application could not boot at all. Storage and behaviour are now separate: CommandsRegistry and PluginsRegistry hold definitions and nothing else, while CommandBinder and PluginBooter do the work and are built later, when the container is complete. Psr\Log\LoggerInterface is not resolvable. Tempest binds Tempest\Log\Logger, which extends it, so that is what the runtime asks for now. Discord is a container singleton rather than something Tempcord alone holds. OptionValueResolver needs it to fetch users and channels, and a plugin may want it too; autowiring it previously tried to construct a fresh Discord and failed on its token argument. tempest/log and tempest/event-bus are now required explicitly. The first was already used and only arrived transitively; the second is needed because tempest/log's own initializer resolves an EventBus without declaring it as a dependency. Finally, console logging could kill the process. Tempest's console parses <style> tags out of whatever it is handed, and these messages carry whatever the gateway and REST layer logged, response bodies included. A long message containing the literal "<style=" exhausts the regex engine's stack, at which point preg_replace_callback returns null, the parser hands that to preg_match, and the process dies on a TypeError. A 401 from Discord was enough to trigger it. Messages are now neutralised and capped before they reach the console. The starter application now boots, registers, binds its commands and opens the gateway.
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
1 participant
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
Splits the framework into explicit declare → compile → run layers, and fixes eight defects found on the way.
The attributes were doing five jobs at once: declaring a command, walking the class to find its parts, building the Discord payload, reaching into the container, and invoking the handler. Everything awkward about the codebase followed from that:
$attribute->reflector = $classhad to be assigned from outside after the fact — every property was a landmine until discovery had run.HasAttributesstood in as an untyped bag.Command::optionsbuilt throwawaynew Subcommand(name: 'fake')instances to borrow it.->optionstwice returned different objects.The lifecycle those getters implied is now explicit:
AttributesCompilerClassReflector+ attributes → immutable definition tree, once, at discovery.DefinitionsCommandDefinitionand friends, with handlers flattened to the dotted paths Discord reports, each carrying the option path it reads from.DiscordCommandBuilderFactory— the only file that touches Fenrir's builders.RuntimeCommandDispatcher,ArgumentResolver,OptionValueResolver,AutocompleteResponder,ChoiceFactory.The user-facing API is unchanged. Bot authors only ever write attributes, and those keep the same names and arguments. This is a pure internals refactor — no BC break for anyone consuming the package.
Bugs fixed
Each has a regression test. The first four were found by reading; the last four surfaced while moving the code.
#[Command]whose__invoketakes no options bound zero handlersifblock duplicating thematcharm below itarray_mapiterated the uncapped array)TypeErrornullresolveFocusedAndParamcompared against a magic[1, 2]ApplicationCommandOptionType#[Option(name: ...)]was undispatchable — args keyed by the Discord name, parameters matched by PHP namenullrather than left absentTypeErroron any non-nullable parameterAutocompleterebuilt on every read of->optionsOne thing I initially flagged turned out not to be a bug: Fenrir's
emitInteractionis a filter predicate by design, so the override here is correct.Other consequences worth reviewing
OptionValueResolvertakes aDiscord, the registries and dispatcher take aContainer,Tempcordtakes its registries. Nothing in the command path callsget()any more — that's what makes the runtime testable without a gateway.Outcomeobjects andBootCommandrenders them, so tests assert on results rather than mockingConsole.AllCommandExtensionandInteractionCallbackBuildermoved intoTempcord\Discord.Review order
The five commits are each self-contained and green; reading them in order is much easier than reading the squashed diff:
fbb0705Bind invokable commands that declare no options9d650cfStop fetching USER options from Discord twiceb4172ceFix autocomplete choice normalisation and unfocused interactions7975454Match subcommand option types by enum case, not magic intsfb65585Split the framework into declare, compile and run layers ← the architectural changeNote that
a7ad3da(Tempest 3 upgrade) was already on this branch before this work and is included in the diff againstmaster.Test plan
composer test— 96 tests, 171 assertions, green (up from 69)composer analyse— phpstan level 5, no errorssrc/, and new coverage exists for the compiler, builder factory, dispatcher, argument/option resolution, autocomplete, both registries, discovery, and an end-to-end path from a bound interaction through to the command method running.Known gap, deliberately not touched
docs/commands.mddocuments an API that has never existed in this codebase — a nonexistentOptionTypeclass,#[Option]on methods rather than parameters, and arequired:argument.README.md:156carries the samerequired: trueerror. Both predate this work (they come from1c92511 "AI Generated documents"). I left them alone rather than half-fix one file; happy to rewrite them against the real API in a follow-up.🤖 Generated with Claude Code