(Code style) To const, or not const - that is the event

Overview

When dealing with internal events (e.g. civi.api.authorize or civi.dao.preDelete), there are two idioms in the codebase -- one based on string-literals, and one based on constants. This issue suggests standardizing on the string literals.

Example use-case

// Using string literals
Civi::dispatcher()->addListener('civi.token.list', $callback);
Civi::dispatcher()->dispatch('civi.token.list', $event);

// Using constants
Civi::dispatcher()->addListener(Civi\Token\Events::TOKEN_REGISTER, $callback);
Civi::dispatcher()->dispatch(Civi\Token\Events::TOKEN_REGISTER, $event);

Current behaviour

Both formulations appear several times in civicrm-core.

Proposed behaviour

Convert the constants to string-literals. Mark the constants as deprecated.

Comments

TLDR: Less is more. The string-literals are simpler and more consistent. The constants are a formality which don't add anything.

My main problem with having the two idioms is that it makes it harder to grep/analyze. Suppose you have the question, "How is event X used?" Answering this question requires that you search on both the literal name and the constant name. Additionally, it makes it harder to learn -- e.g. you may find that documentation and examples are inconsistent in the formulation, prompting the newcomer to ask: "Why are these different?"

Of course, the "two idioms" could be resolved either way -- standardize on string-literals or on constants. IMHO, the two approaches are about equal wrt grepping/analysis - trading some marginal differences. Some IDEs support PHP-sensitive "Find Usages" (which works better with constants); but if you don't have that, then searching the codebase for the constant is more quirky (e.g. ::EVENT_NAME is not distinctive; e.g. class-names can be aliased). The string-literals are not quite as slick as IDE "Find Usages", but they are more distinctive, and they're searchable in any tool.

So that parts a draw... why are string-literals better?

• At a systemic level, the constants require superfluous class-loading. Consider something like Civi\Core\Container:;createEventDispatcher() which runs on every page-request and which currently registers (ballpark) ~30 listeners. Most of those listeners won't be used in a typical page-request, but you'd have to the load the classes just to resolve the constants during initialization. As you grow more committed to loose-coupling/modularization (e.g. dispatching and subscribing to events in more situations), the superfluous loads grow worse.

• The constant is an extra layer/add-on. We've already been lax about using them civicrm-core. You can hardly expect third-party extensions to follow the convention it. (On the flip-side, if the constants don't exist, then everyone will just use string-literals by default.)

• The constants are usually longer+messier to type than the underlying string (Civi\Token\Events::TOKEN_REGISTER vs civi.token.register).

• The string-literals are usable in lots of places where the constants wouldn't be, e.g.

  • In services.xml or services.yml files
  • In dev tools like cv debug:event-dispatcher
  • When setting breakpoints and inspecting runtime data
  • In soft/optional integrations. (Ex: Suppose CiviVolunteer bundles enhancements for CiviRules, but it doesn't actually require CiviRules. Thus, the CiviVolunteer would have references to some CiviRules events, but the CiviRules classes and constants may not actually be valid in a particular deployment.)

Are there reasons to prefer constants over string-literals? Theoretically... with constants, you can change the value! That has some utility for, eg, CHECK_USERNAME_TTL - where the constant's value is a bit arbitrary and its impact/usage is isolated. IMHO, this does not apply to event-names. Event-names are part of the contract between different pieces of code. You should only change the name to signify a semantic break in the contract - and if you're doing that, then you need to reconsider/update the listeners anyway. The only changes facilitated by the constant are silly typographical changes. (But even that's a wash - perhaps it's easier to twiddle the content of the string, but you've added a new symbol which may also need twiddling - and it's just as hard to twiddle that one.)