Skip to content

Feedback fixes - #330

Merged
jaydevb merged 84 commits into
stagefrom
develop
Aug 25, 2026
Merged

jaydevb merged 84 commits into
stagefrom
develop

Conversation

@jaydevb

@jaydevb jaydevb commented Aug 19, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

jaydevb and others added 30 commits August 3, 2026 16:49
content_analytics already self-throttles via sync_frequency=daily
config check, so the 15-min trigger was redundant; move to daily.
tmgmt_cron moved to 30-min. node_cron job removed — no node_cron()
hook exists in this Drupal core version, job was dead config.
…ter removal, orphaned-revision fix) at user request
Menu links are content entities, so config import never applies menu changes to a site. Each site drifted independently: 40 links on stage, 43 in config, 39 in structure_sync, with per-role visibility differing on 21 links and titles, paths, weights or parents differing on 33.

Adds a canonical menu in config plus two Drush commands over one service: bebbo:menu-export writes the current menu into config, bebbo:menu-sync applies it to a site and is idempotent. Deletions are scoped to editorial-menu, and parents are created before their children.

This replaces menu_export, whose exporter reduces every multi-value field to its first value and so collapsed links visible to four roles down to one.

Country Users no longer hardcodes a group ID. Group IDs are per-site auto-increment values, so one number pointed at a different country on every site, or at nothing: the link 404'd on five of seven sites. It now targets /country-users, which resolves the acting user's own group at request time.
menu_export.export_data was only read by two update hooks that are stamped on every site and can never fire again, so it applied to nothing while still looking authoritative. Both hooks already handle its absence.

structure_sync.data carried a third copy of the menu, 39 links with no per-role visibility at all. Nothing invoked it, but its import form is one click away and its delete step is not scoped to a menu, so a Full or Force import would wipe links across every menu and drop the role restrictions the menu depends on.

Editorial entries are stripped from the shared file and from the somoa and zimbabwe split patches; the quick-links, main and footer entries stay. Split sequence keys hash each item's uuid, so the remaining patch keys still resolve.
Numbered update hooks stamp once and never fire again, so menu changes reached config but never the menu links. Running the sync after config import means every deploy converges each site on the canonical menu, and a menu change needs no new hook number.
Covers the bebbo_custom_general.editorial_menu config object, the editorial_menu_manager service, the /country-users route, the bebbo:menu-export and bebbo:menu-sync commands, the module's newly declared group and menu_per_role dependencies, and the extra deploy step in code-deploy.sh.
Standing up a new site left operators guessing: the README covered three features and the rest of what has to be configured before AI, MFA, analytics, content sharing or the secured API do anything was undocumented. POST_SETUP_CONFIGURATION.md now covers each of them — what is shipped in config, what must be entered, where, which permission gates it, and what has to be true for the feature to actually work.

The Microsoft 365 steps described the wrong OAuth flow. The module requests delegated IMAP.AccessAsUser.All, SMTP.Send and offline_access over SMTP and needs an interactive sign-in per environment, not the Mail.Send application permission. Registering the per-host redirect URI was missing entirely, which is the step that fails first.

Both API-security environment variables are now stated as Acquia Cloud settings, per environment, rather than only mentioning the JWT key in passing.

PHP claims corrected across README, ENVIRONMENTS, CICD_DEPLOYMENT and RUNBOOK: the workflow pins 8.4 for all three jobs, not 8.3 for the stage deploy, and that version is the runner's rather than the Acquia environment's.

API_REFERENCE gains the Strings endpoint, which was listed but never described, and a verified V1 health-checkup response. Both date fields are rendered <time> markup on every endpoint, not the plain strings the examples imply.
…dation on course link

The expiry date validator crashed with a TypeError when the user submitted
a date without a time — the datetime widget returns an array instead of a
DrupalDateTime object. Now detects the array case and shows a form error.

Also adds URL format validation on the Course Link field, which previously
accepted any string.
The setting was defined in install config, config schema, the exported
config and the admin form, but SecurityController::refresh() never read
it. The endpoint is publicly routed, so token refresh ran unthrottled
while the other three public endpoints were flood-guarded.

Key the flood on the device that owns the presented token rather than
the client IP: mobile carrier NAT puts many real devices behind one
address, and a 30/hr per-IP ceiling would reject legitimate traffic.
The lookup ignores revoked and expired rows so replay attempts count
against the same device budget instead of escaping the limit. Tokens
matching no device fall back to the client IP so forged values cannot
be sprayed for free.
Policy permits only security-related user-facing mail. Moderation
notifications are already off and password reset plus the Email TFA
OTP cover the permitted flows, so the account-created notice was the
one remaining outbound message beyond that boundary.

Registration is admin_only, so account creation now requires setting a
password on the user-add form and delivering it out-of-band.
The refresh endpoint is now flood-guarded, so it belongs in every
place that enumerates the rate limits rather than being listed as an
admin-form-only setting. The email policy drops to two user-facing
flows, which changes what an admin has to do when creating an account.
The sync log view used the tag cache plugin, but its base table is a
plain custom table with no entity type, so Views derived no cache tags
for it. The stored result set carried max-age -1 and only the view's
own config tag, which fires when the view definition changes and never
when a row is inserted.

The logs page therefore served a permanently frozen result set: a sync
would write its row and stay invisible until an unrelated cache clear
happened to flush the entry. Cron rows were affected the same way and
only appeared because routine clears fell between weekly runs.

An admin-only page reading thirty rows from one indexed table gains
nothing from caching, so drop the plugin rather than invent a custom
tag and a views alter to attach it.
Editors who set only a date got "The Course Expiry date is invalid" from
core, because the datetime element builds its value against "Y-m-d H:i:s"
and a date-only string cannot satisfy that format. The previous fix caught
the leftover array in a validator and raised a friendlier error, but the
editor still could not save.

A date on its own reads as "expires at the end of that day", so supply
23:59:59 rather than refusing the input. It has to happen in the value
callback: by the time any validator runs, core has already failed to build
the date and left the raw array in place, and nothing a validator writes
would reach the saved value.

The client-side fill is convenience only, so the time is visible while
editing instead of appearing from nowhere on save; the server applies the
same default either way.

Also restricts the course link to http and https. FILTER_VALIDATE_URL on
its own accepts any scheme carrying an authority, ftp:// included, and app
users can only follow a web link.
The field gave editors no indication of what the date governs, which is
why a date-only entry seemed reasonable in the first place.
Filling the time only when it was blank left editing an existing course
untouched, since a stored expiry always carries a time. Changing the date
kept the old one, so re-scheduling a course to a later day silently
inherited whatever hour it had been given before.

A course expires at the end of its last day, so the time follows the date
rather than being chosen alongside it. Clearing the date is exempt: a time
with no date fails validation, so it must not leave one behind. Emptying
the time on its own remains a way to ask for end of day.

The server still defaults only the empty case. A submission carries no
record of which field the editor touched, so it cannot distinguish a
changed date from an unchanged one.
Force Update took its Google Play and App Store links through plain
textfields behind an empty validateForm(), so any string at all was
stored. App Store Redirect used a url element, which checks the format
and nothing else — the default site currently points iOS users at
google.com, which is how the gap surfaced.

Validating the Force Update entry form alone would not have been enough.
Its values travel to the confirmation step as query parameters and are
inserted from the query string, so the persistence point accepts whatever
it is handed regardless of what the entry form allowed. Both now check,
and the confirmation step drops a bad link rather than refusing a submit
the editor cannot correct from there.

The rule lives in bebbo_custom_general, whose stated purpose is shared
utilities, so the two forms cannot drift apart on what counts as a store
link. That makes pb_custom_form depend on it; both are already enabled on
all seven sites, so core.extension is untouched.

Host alone is not sufficient — play.google.com serves developer pages and
search as well as listings, and a lookalike host would otherwise pass on a
suffix match. Listings are identified by their path: /store/apps/details
carrying an id parameter, or an /id<digits> segment for Apple. https is
required, since neither store serves plain http.
Feature Setup duplicated the AI, Office 365 mail and API security steps
that POST_SETUP_CONFIGURATION.md already documents in more depth, so the
two drifted apart. README now states these are settings required after
standing up a new site and links the guide; the README-only details
(OpenAI key generation, Entra secret caveat, JWT keygen commands) move
into the guide so it stays the single source.
Entity storage only loads dedicated-table field values whose langcode is
an installed language, so a user left pointing at a removed language -
en-gb on Zimbabwe, the old shared langcodes on Pakistan - loaded with no
roles, no allowed languages and no picture while the rows sat on disk.
The account form therefore showed nothing selected, and saving rewrote
the values under the same invalid langcode, dropping whatever the form
could not display. Accounts flagged administrator in the database were
plain authenticated users at runtime.

Repairs the affected users and their field rows to the site default
language. Runs on every site through the per-site updb in the deploy
hook.
Views data export displays that write to private:// failed to save with
"invalid export filesystem scheme: private" on DDEV, because
file_private_path is set only inside the Acquia branch of
post.settings.php and nothing supplies it locally. Putting it in the
generated settings.ddev.php does not stick - DDEV rewrites that file.

common.settings.php is required by every site through
docroot/sites/default/settings.php, so one guarded block covers all
seven, and post.settings.php still overrides it on Acquia.
Admins had no way to discover the public /downloadapp.html page that the
configured store URLs feed, so the link is now shown on the settings form
and repeated in the save confirmation.
The langcode repair hook wrote fully qualified class names inline, which
the Drupal coding standard rejects.
The transport entity shipped user/client_id/client_secret/tenant_id as
explicit null. TransportBase::setConfiguration() unions the stored
configuration over the plugin defaults, and a stored null wins that
union, so the plugin's defaultConfiguration() - the only code that reads
the real credentials out of symfony_mailer_office365.config - never
applied. Every cim rewrote all seven sites with nulls, the DSN came out
credential-less and EsmtpTransport::setUsername() rejected the null,
breaking all outgoing mail.

Omitting the four keys lets the defaults through. Key-level config_ignore
keeps a manual save of the transport form from baking the resolved
secrets back into the entity, where a later export would commit them.
jaydevb and others added 28 commits August 19, 2026 13:44
Drupal 11 removed the legacy 'replace' value for #ajax method, so
selecting a different scheduler or logger threw "$wrapper[method] is not
a function" and the settings pane never re-rendered - the job form was
effectively read-only for plugin type.

Upstream fixed this in 8.x-2.x but has not tagged a release since, so
carry the merge request as a patch.
The job carried an hour-range rule under the Simple scheduler, which
only understands its ten fixed presets - the listing page warned on the
missing key and the label was unreadable. Crontab parses the rule the
job was always written for.

Two hours matches the intended edge TTL: a pass finishes and the next
one starts before the warmed responses expire.
Cron is driven by the scheduled jobs on the hosting side, one per site.
Letting a visitor's request trigger it as well means a warm pass or a
purge drain can start inside an anonymous page load, on whatever site
happened to be hit first.
Core marks every cacheable response Vary: Cookie, which is correct for
HTML - an editor with a session and an anonymous visitor must not share
a cached copy. The API endpoints are anonymous JSON whose body does not
depend on any cookie, so there the header declares a dependency that
does not exist, and shared caches believe it: a request carrying any
cookie cannot reuse the copy stored for a cookie-less one, so it misses
and pays a full origin render for a byte-identical response.

The mobile app sends no cookies today, which is the only reason the hit
rate survives at all. Anything that starts attaching one - an analytics
cookie, a browser hitting the endpoint - would fork the cache silently.

Scoped to /api/* and the versioned prefixes so HTML keeps the header it
needs, and only Cookie is dropped: Accept-Encoding and anything an edge
added survive.
DrupalPractice flags global constants, and these two were declared at the
top of the .module file. They describe when a node save leaves the API
responses alone, which belongs with the rest of the cache-tag code.

Behaviour is unchanged - the same two lists, read through class constants
instead of global ones.
Opening a translation job with no provider chosen makes the job form
preselect whichever provider sorts first and probe it for supported
languages. Where that lands on Google, an expired or restricted API key
printed a page-level error over a job the editor intends another
provider to translate - the message appeared twice, named Google, and
had nothing to do with the work in front of them.

Log the failure instead, and put it on screen only on Google's own
settings form, where the reader is asking about Google. The provider
stays enabled and usable; nothing about translation behaviour changes.
The CDN warmer sent no timeout with its requests, so Drupal's http_client
default of 30 seconds applied. A cold non-English /api/* render takes longer
than that (measured 34.5s against 1.4s warm), so every attempt aborted with
cURL error 28 and those languages were never warmed - the same failure
repeated on every run.

Patch the warmer to allow 5 minutes per page, with a short connect timeout so
an unreachable host still fails fast.
/country-reports and its CSV export granted access to the authenticated role,
so every logged-in user reached them regardless of the hidden menu tab, and
the standard deviation export ran on the 'access content' permission, which
anonymous holds - the page and its export were public on every site.

Both now use the same role list, with editor added because editors are meant
to have these two reports. Editorial menu links are content entities, not
config, so the matching menu visibility is applied by an update hook; it walks
up the parent chain, since a child link stays hidden while an ancestor is, and
skips links with no roles set, where adding one would restrict rather than
widen.
The warmer spent every cycle requesting languages that return an empty
payload. Verified against production across seven endpoints per language:
English is empty on every country site (their content is served under the
country langcodes), Spanish is empty on Ecuador, ws-sm and fj-fj are empty on
the Pacific site, and ro, sq and sr are empty on the default site. Languages
that are seeded but not yet populated stay in the list, since they will fill
in over time.

Raise concurrency from 2 to 8 - the same fourteen URLs took 23 seconds at 2
and 10 seconds at 8 - and send a header so Cloudflare can be told to bypass
its cache for these requests. Without that bypass the warmer is served by the
edge and never reaches the origin it is supposed to keep warm.

Disable the enqueue-only cron job: a single scheduled command per site,
"drush warmer:enqueue cdn --run-queue", both fills the queue and drains it,
which the cron path cannot do because the queue worker is capped at 60
seconds per run.
The update hook that widened these links ran at updb, but the editorial
menu sync runs later in the same deploy and rewrites show_role from the
canonical config. The hook's grant was therefore erased on every deploy
that applied it.

Putting the editor role in the canon lets the existing menu sync carry it
to all seven sites, so the hook is no longer needed.
The user listings reached group membership through a reverse
group_relationship relationship and rendered the group columns from it.
That table is one-to-many in two directions at once: a user holds one
membership row per group, and groups_field_data holds one row per
translation. Both multiply the result set, so a member of a single
translated group was already listed several times, and every extra group
multiplied it again. Zimbabwe rendered 168 rows for 45 users.

Enabling the query's distinct option does not collapse this. Views adds
the related entity IDs to the SELECT so it can load those entities, and
those IDs differ per row, so the duplicates are never identical. That is
why the same option worked on the People page, which carries no group
relationship, and does nothing here.

Read the memberships outside the query instead: a Views field that lists
every group of a row's user, and filters that match membership through a
subquery. No join, nothing to multiply. Memberships for the whole page
are loaded in one query, and the subquery restricts groups_field_data to
the default translation.

The exposed Country filter keeps its identifier, and the group scoping
of the country listing is unchanged: verified on all seven sites by
capturing the visible user IDs before and after, with no user gained or
lost for any viewer, and every rendered group cell compared against the
membership table.

The user reports view is deliberately left alone. It carries four
group-derived columns and its CSV export may rely on the row per group
shape, which is a separate decision.
The user listings no longer reach group membership through a relationship,
so note the Views plugins that replaced it and the row per user behaviour
they give, and keep the users reports view described as it still is.
The eight translator entities were hidden from config entirely, so each
site kept its own copy in its database and no commit could reach dev,
stage or prod. They are shared config now, which puts their existence,
label, weight, plugin and status under review like everything else.

Their remote language maps cannot be shared. Each site serves a
different set of languages, from twenty eight on the default site down
to two, and the map is what tells a provider which remote code answers
for each of them. The shared base therefore ships an empty map and each
site supplies its own through a partial split patch, generated from what
that site is running today.

Credentials stay out of the repo. The shared files carry blank key
fields, and config_ignore keeps only the credential keys site owned, so
an import cannot overwrite a live API key with a blank one. Nothing
else about the translators is ignored any more.

Also drops eight patches that pinned the file and user providers to
their own weights on four sites. They fought the shared order, and the
values they claimed to be replacing no longer matched the shared config.

Verified on all seven sites by importing and then comparing every
translator against what the site held beforehand: no map lost a key, no
credential was cleared, no entity was created or recreated, and a
second import leaves all of it alone.

Note for deployment: the uuids in config match the live entities
locally, but an environment where they do not will make Drupal delete
and recreate the entity, and that delete walks the site's translation
jobs. Run scripts/tmgmt-translator-uuid-align.php once per site before
the first import in each environment.
The translator section still said no translator config exists in sync
and that the whole entities are ignored, which is now the opposite of
what the repo holds. Record where each part lives instead: the shared
entities and their order, the per site language maps, the credential
keys that stay environment owned, and the uuid alignment a new
environment needs before its first import.
Every site's translator patch carried the default site's full European
language map, inherited from the databases the splits were captured from.
Sites paid for mappings they can never use, while their own languages
fell back to raw langcodes the providers do not recognise.

Trim each patch to the languages that site actually has, and fill the
gaps that leaves: an empty value where the provider has no such
language, the base code where it does.
Drupal 11 renders GUI requests inside Fibers, which makes the OpenAI
provider force-stream every chat call. The streamed iterator flushed
its buffer every ~100 bytes through HostnameFilter, whose per-fragment
HTML5 parse/re-serialize auto-closed open tags and dropped the real
closing tags, corrupting translated body HTML in both the AI Translate
and TMGMT paths.

Apply upstream MR !1734 (drupal.org issue 3586558) as a composer patch
on drupal/ai 1.4.3: hold the flush buffer until every open HTML tag is
balanced, with a 100KB ceiling against never-closing tags.
The patches table listed 30 of the 39 applied patches; add the nine
missing rows and correct the counts. All 31 local patch files verified
present on disk.
Replace the 39-link editorial menu with the approved 36-link structure:
new parent order, Users parent dissolved into top-level Add user and
User List, three new tabs (Add Translation Job, Add taxonomy feed
types, Add taxonomy feed), Google Analytics added disabled (module not
installed), seven obsolete tabs removed and five renamed. Roles follow
the spec, with administrator granted on every item and authenticated
on the two report tabs. Menu link UUIDs are preserved for surviving
items so re-sync updates in place instead of duplicating.
The editorial-menu preprocess wipes every menu item for users outside
the target-role list who have no group membership, so translators
without a country group saw an empty editorial menu. Translators are
editorial users; include them so the menu and the trusted-role row
operations on the content listings apply to them.
@jaydevb
jaydevb merged commit 3fdf4ec into stage Aug 25, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant