Writing good WebEngine code #742
g105b
started this conversation in
General chat
Replies: 0 comments
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Over the last few weeks I've had the pleasure of working with some developers who were new to WebEngine, and it's always fulfilling to see their delight at the simplicity of some things. There are, however, many common pitfalls that are fell into over and over. I've written this guide to help guide human developers, and automated tooling, to writing good quality WebEngine code. It's a bit long, and definitely not designed for bedtime reading, but hopefully it helps someone increase their code quality over time.
I will add to this as and when, and once I'm happy with it it will go into https://github.com/phpgt/WebEngine/wiki/Coding-styleguide
Build practical WebEngine applications whose behaviour can be followed directly from HTML to page logic, owning classes and persistence. Good structure supports that traceability; architectural complexity is never a goal.
Scope and architectural judgement
Readability and ownership
AuthServiceif there's already a class that deals with authentication - just use it directly.WebEngine page logic and components
delivering functionality, put business rules in their existing owning classes
in
class/and unit-test new functionality, edge cases, invalid input andmissing data before browser integration. Simple page orchestration stays inline;
the sketch is not a reason to introduce a class for every operation.
page/only definesgo*anddo_*functions. Keep these light:read input, orchestrate typed services, bind results and choose an HTTP response.
Keep HTTP guards and presentation decisions here; business rules belong in
their owning classes.
page/_component, withits own PHP action handlers. Small markup fragments do not need a component
merely because they appear on more than one page.
name="do" value="example"maps todo_example(). Component controls normally use their component's own handlers;do not assume a same-named function elsewhere will run. Check WebEngine's
component routing and lifecycle when a form posts to another page. A generated
__componentfield must not prevent a deliberate page-level submission fromreaching its actual handler.
in its caller; put elements such as
aside,navorsectioninside it ratherthan adding a caller-side wrapper solely around that one component. Keep outer
containers when they genuinely group or lay out multiple elements.
Test page/component behaviour with Behat, not direct page-function unit tests.
data-listfor repeated data. Prefer one readabletemplate for existing and new records to near-identical copies for each state.
Fields may remain present but hidden in the relevant state. Read-only fields
still need meaningful
nameattributes.run after their data has been bound. Consult the framework lifecycle rather
than adding caches, duplicated data attributes or guessed execution hooks.
_header.htmlor_footer.htmlfiles. These extend the outer magic header/footer; do not repeat the same
component in every page within a directory.
small stateless helpers in page functions when useful; register reused or
stateful services in
ServiceLoaderfor typed parameter injection.Types, binding and persistence
adapters at library/database boundaries may translate data when necessary.
represent the same value. Prefer
data-bind:value="@"(the shorthand for@name) instead of repeating a matching bind key. Human-readable labels candiffer without renaming the data: a deployment's
titlecan be shown as"Deployment name" without translating the field through several layers.
context, managed, panel or settings classes merely to copy fields, rename them
and unwrap them again.
Keep small presentation calculations, select options, disclosure state and
rejected form values in the component's binding callback. Use existing binder
callbacks before creating a view-model class. A distinct projection must solve
a real transformation or business requirement, not keep duplicate shapes in sync.
name, including in migrations, queries and fixtures. Entity properties reflect
the table's columns, types and nullability. Preserve persisted values during
hydration; apply display fallbacks in presentation code. Keep UI projections
distinct from entities. Do not add ORM dependencies,
mapping attributes or persistence abstractions before ORM integration is needed.
readonly, and usereadonly classfor other classeswhose properties remain fixed after initialisation. This expresses the intent
of services as doers with fixed dependencies; they can still update the
database, session or other objects they hold. Readonly value objects are useful
too. Remove redundant property-level
readonlyinside a readonly class.Keep classes mutable when their own properties change, and respect inherited
framework constraints. Do not introduce state wrappers just to obtain readonly.
needed. Use the canonical namespace casing declared by the installed package
(
GTfor migrated PHP.GT libraries); do not depend on compatibility aliases.Do not add subclasses, casts or state wrappers to conceal a misunderstood API.
getBy*lookups return nullable objects; handle absence explicitlyat the caller. Alternate-key lookups use
a single-row
getIdBy*.sqlquery withfetchString/fetchInt, then delegate tothe canonical ID lookup rather than duplicating object hydration.
for those entities rather than adding separate action wrappers. Return useful
values from application services (for example, a redirect
Uri) and performthe HTTP redirect in page logic.
actions. A single-method class is not inherently wrong, but its responsibility
must justify the class beyond giving a function a name.
missing, add a numbered SQL migration in
query/_migration/and rungt migrate.Do not substitute session-only demo behaviour for persistence.
Auditing and security
authenticated or anonymous actor and concise changed values for state changes.
Do not add separate forwarding services or payload classes without a concrete
need. Never record credentials, including passwords, API secrets, ciphers or session IVs.
Commit application mutations and their audit records together. Cover audit rows,
rejected input and audit-write failures in unit tests and the browser flow.
should be concise; do not create a generic diff factory or copy entire request
payloads into every event. Mutation records identify the entity, actor and
meaningful
from/tochanges without repeating unchanged snapshots or nestingredundant envelopes. Keep auditing in its owning class instead of adding
forwarding services, action wrappers or payload classes.
Across client/provider boundaries, identity must come from authenticated data
and be checked against its deployment; a plain user ID in a query string is not
proof of identity. Do not assume the provider retains a session after sign-in.
HTTP flows, forms and progressive enhancement
enhancement after the plain HTTP behaviour works.
After a logout or other completed POST action, return to an ordinary GET
destination. Do not append action parameters such as
do=logoutthat re-entera client's handler and create a loop. Preserve intentional destination paths,
queries and fragments without introducing new actions.
required, inputtype,pattern,minlength/maxlength, ranges and steps as appropriate. Validate the same formserver-side with
phpgt/domvalidationbefore performing mutations. Do notmaintain a second set of field-format or length rules in PHP classes.
Use labels and
titlehints to explain constraints, and show server errors atthe submitted form without discarding its values. PHP still enforces ownership,
permissions and other business rules that HTML cannot express.
before mutation. Classes may normalise validated values; ownership, permissions,
uniqueness and other business rules remain in their owning PHP classes.
reasonable assumptions instead of encoding an entire networking specification
in a pattern. Add complex formats such as IPv6 only when required. Patterns
must work in both HTML and DomValidation.
browser validation is deliberately bypassed. Check library/browser parity for
relevant edge cases such as Unicode lengths and the string "0". Fix validation
library defects in the library, rather than adding application workarounds.
Repeating form templates are extracted before handlers run; retrieve the
canonical form through
ListElementCollectionwhen validating adata-list.at the submitted form. Make the error visible and focusable where appropriate.
A validation failure must not silently discard input or appear to do nothing.
Derive enhancements from existing markup rather than duplicating link targets
in extra attributes. Generated duplicate links must not add extra tab stops.
controls inside new links. Generated duplicate table links use
tabindex="-1";preserve the original link as the keyboard entry point.
Shared design and accessible controls
in the theme. Reuse its patterns before adding local CSS. Cover normal, hover, focus, active
and disabled states with readable contrast, including primary controls.
Fix cascade/specificity problems rather than adding
!important. Preventinitial-render flashes and layout jumps with HTML/CSS before Flux runs, while
preserving usable controls without scripting.
including temporary previews, share geometry, animation and reduced-motion rules.
Fix a CSS/rendering problem in that shared code instead of creating a separate
variant or editing/generating replacement SVGs to compensate for it. Preserve
supplied icon assets unless an asset change is explicitly requested.
.actionscontainer using the design library'sshared spacing: the top space should match an input caption's line height and gap.
Keep single panels at a readable maximum width; use responsive layouts that
preserve the meaningful HTML order. Use native exclusive
disclosures when only one item should be open.
the intended Save/Continue submit button first so Enter submits that action;
position secondary or delete controls with CSS rather than moving them ahead
of the default submit button in HTML. Icon-only controls retain an accessible
name through screen-reader text and use the shared icon-button pattern.
Honest functionality and destructive actions
handlers, fake records, unused dependencies and their tests when real behaviour
replaces them; clearly mark unfinished controls as unavailable.
Define what observable event completes a milestone, including production and
development exclusions, then persist and audit it. Do not infer completion
from an unrelated setting or confuse a development exemption with a completed
production milestone. Be explicit about whether earlier events can be recognised; do not invent or
backfill historical success without reliable evidence.
identity is user-facing; a deployment label identifies an environment. Keep
those meanings distinct even when their current strings happen to match.
Keep an HTTP confirmation fallback and require confirmation for the same
specific target on the server before deleting. Cancel and Escape must never
perform a deletion.
Tests and database isolation
snapshot their database, apply dev-only fixtures in
test/behat/database/<state_name>, and restore after the run, including failures.test/phpunitand browser flows intest/behat. Reuseexisting Behat steps and describe observable behaviour using meaningful UI
elements, such as the application switcher, rather than incidental fixture
words. Test real routes, persistence and audit records rather than a simulated
backend. Check HTTP and Flux behaviour where enhancement affects the flow.
relevant content or a completed request. A shared sidebar appearing does not
establish that a redirect chain or background form submission has finished.
Fix the observed timing condition instead of masking races with arbitrary
sleeps or retries that can conceal a real failure.
XTest, whereXis the class name. Mirror the source namespace/directory. Consolidate tests
by class, not by purpose: test
AdminAreainAdminAreaTest, never in aseparate
AdminAreaSecurityTestorLogoutTest.test+ the tested method in PascalCase, optionally followedby
_and a descriptive context:testLogout,testLogout_newSession, ortestLogout_invalidSessionShouldThrow. UsetestConstructfor constructors.Name data providers and fixture helpers for the data they provide; they do
not use the
testprefix.$sut.Keep its construction, exercise and assertions easy to follow. Use data
providers for cases needing different subject instances rather than replacing
$sutor constructing subjects in a loop. Assert the subject's behaviour,not several unrelated classes' behaviour in one test. Several assertions may
establish one result; one system under test does not mean one assertion.
to the behaviour. Keep real value objects when their values are needed; avoid
unnecessary databases, transports or application services in unit fixtures.
Preserve checks that require actual SQL, files or several services in the
separate
test/integrationPHPUnit suite, included incomposer test.Class-based integration tests may mirror the corresponding unit-test class
names under their own namespace. Existing page-function integration checks
are retained there; write new page/component coverage in Behat.
composer testgreen with PHPUnit, Behat, PHPCS, PHPMD and PHPStan. Whensimplifying code, update tests to protect supported behaviour; do not restore
unnecessary implementation just to satisfy an obsolete test.
proportionate to the change and choose tests for their ability to catch a
concrete defect, not to increase a count or preserve discarded architecture.
tests that repeat assignments, demonstrate framework behaviour or check routine
service wiring already exercised by real flows. Test count is not a measure
of useful coverage.
Verification, dependencies and review
schema behaviour, a browser for rendering and initial-paint behaviour, and the
built/installed assets for package changes. State what the checks establish
and any material limits; a substitute environment passing is not proof of
behaviour it cannot reproduce.
under the same runtime conditions, inspect the actual output and establish a
cause. Do not successively guess at CSS sizes, origins or other settings and
present each guess as a verified fix.
page. Check relevant viewport sizes and themes; inspect initial paint for
flashes and frame-by-frame output for blur or movement when necessary.
Functional browser assertions and a passing asset build do not establish that
an animation looks correct. State any visual checks still outstanding.
constraints, lockfile, installed
vendorcode and built/installed npm assets,not merely a sibling source checkout. Verify the complete integration contract:
parameter names and meanings, encryption, redirects, session persistence and
cookie flags in the actual HTTP/HTTPS environment. Do not add speculative
backwards compatibility to paper over mismatched client/provider versions.
unrelated locked versions. Update affected build commands and configuration,
then check that the configuration is actually honoured at runtime. Fix library
defects in the owning library instead of committing vendor edits or creating
application adapters around them.
changing behaviour. For DNS, distinguish the zone, record name, fully qualified
lookup name and TXT value. For client protocols, distinguish application IDs,
deployment IDs, destinations and action parameters. Do not guess which value
the maintainer meant or silently swap their meanings.
inspect changes across SQL, PHP, templates, selectors, fixture values and prose.
Matching text is not necessarily the same identifier or responsibility. A
replacement count or green test run does not establish a correct refactor.
maintainer would, preserving unrelated existing edits. Check for unnecessary
files and layers, duplicated responsibilities, misleading names, dense logic,
stale references, obsolete code and convention violations. Remove problems
found during this review, run the checks appropriate to the final changes,
and report the result. Passing tests alone does not make the work complete.
All reactions