Repository navigation
Add MkDocs documentation with GitLab Pages CI - #69
Conversation
jjroelofs
left a comment
There was a problem hiding this comment.
The MkDocs site builds successfully with CI=true mkdocs build --strict, and all GitHub checks pass. I found eight actionable documentation accuracy issues below. Several runnable examples currently call unavailable APIs or point to nonexistent routes, so these should be addressed before merge. GitHub does not allow an author to request changes on their own PR, so I am submitting this as a comment review.
| ## Module settings | ||
|
|
||
| After enabling the module, visit **Administration > Configuration > System > | ||
| RL Settings** (`/admin/config/system/rl`) to configure the module. |
There was a problem hiding this comment.
[P1] Point users at the registered settings route. rl.routing.yml defines this form at /admin/config/services/reinforcement-learning under Configuration > Web services, so the documented URL returns a 404. The same stale URL appears in docs/getting-started/installation.md:10; please update both occurrences.
| drush rl:config:get exploration_rate | ||
|
|
||
| # Set a specific setting | ||
| drush rl:config:set exploration_rate 0.1 |
There was a problem hiding this comment.
[P1] Use a setting that the command accepts. RlConfigCommands::SETTINGS_SCHEMA only contains debug_mode, enable_event_log, event_log_max_rows, and chart_line_threshold. Both exploration_rate commands return Unknown setting (and 0.1 is not a valid value for any current numeric setting), so readers cannot run this quick-start example.
| $exists = $registry->exists('my-experiment'); | ||
|
|
||
| // Remove an experiment and all its data. | ||
| $registry->delete('my-experiment'); |
There was a problem hiding this comment.
[P1] Use the public registry/manager methods. ExperimentRegistryInterface exposes isRegistered(), not exists(), and it has no delete() method. Running this sample therefore fails on both calls. The current cleanup API is rl.experiment_manager->purgeExperiment(); please update the example accordingly.
| composer require npm-asset/plotly.js-dist-min:^2.35 | ||
| ``` | ||
|
|
||
| This installs the library to `web/libraries/plotly.js-dist-min/`. |
There was a problem hiding this comment.
[P1] Remove the obsolete external Plotly installation. The rl/plotly library loads js/vendor/plotly.min.js, which is already shipped by this module; no code reads web/libraries/plotly.js-dist-min/. These steps unnecessarily add a Composer plugin and alter the consuming project's installer paths even though the installed asset is unused.
| - **Security modules**: whitelist `/modules/contrib/rl/rl.php` | ||
|
|
||
| If server policies prevent direct access to `rl.php`, use the Drupal | ||
| Routes API instead. |
There was a problem hiding this comment.
[P1] Do not offer a route fallback that the module does not provide. rl.routing.yml contains only admin report/settings routes and no tracking API route, so a site that cannot expose rl.php has nothing named “Drupal Routes API” to switch to. Either document that consumers must implement their own route or provide/link to a real module endpoint.
| ``` | ||
|
|
||
| All three sections are optional. Invalid or unregistered entries are | ||
| dropped silently so one bad event does not poison the rest of the |
There was a problem hiding this comment.
[P1] Document batch rejection responses. Batch failures are not silent: handle_batch_request() returns an errors array, and rl.php responds with HTTP 422 and ok: false when every requested entry is rejected; Drupal.rl also surfaces those errors in the console. Please describe the errors response and add 422 to the error table so API clients do not assume only 400/500 are possible.
| services: | ||
| my_module.experiment_decorator: | ||
| class: Drupal\my_module\Decorator\MyExperimentDecorator | ||
| arguments: ['@entity_type.manager'] |
There was a problem hiding this comment.
[P1] Make the example service instantiable. MyExperimentDecorator above has no constructor and accesses the entity type manager statically, but this definition passes @entity_type.manager to its constructor. Symfony will fail to instantiate the copied example. Remove the arguments line, or add constructor injection and use the injected service in decorateArm().
| returning visitors tune them out, seasonal calls to action. Leave them | ||
| running. RL follows the winner as it shifts. | ||
|
|
||
| In both cases the loser of a pair just stops receiving traffic on its own, |
There was a problem hiding this comment.
[P2] Do not claim Thompson Sampling stops loser traffic. Each decision draws again from every arm's Beta posterior, and the implementation explicitly preserves exploration, so a losing arm retains a nonzero chance of selection. It generally receives less traffic; it only stops once the experiment owner removes or locks the variant.
| drush rl:config:list | ||
|
|
||
| # Get a specific setting | ||
| drush rl:config:get exploration_rate |
There was a problem hiding this comment.
Nonexistent config key: exploration_rate does not exist anywhere in the module. config/install/rl.settings.yml and the allow-list in RlConfigCommands define only debug_mode, enable_event_log, event_log_max_rows, and chart_line_threshold. Both documented commands (rl:config:get exploration_rate here and rl:config:set exploration_rate 0.1 on line 20) fail key validation, and the doc implies a tunable exploration knob the algorithm does not have. Use one of the real keys in the examples.
| After enabling the module, visit **Administration > Configuration > System > | ||
| RL Settings** (`/admin/config/system/rl`) to configure the module. |
There was a problem hiding this comment.
Wrong settings path: the real route (rl.settings in rl.routing.yml) is /admin/config/services/reinforcement-learning, titled "RL: A/B Testing", and rl.links.menu.yml places it under Web services, not System. /admin/config/system/rl returns a 404. The same wrong path appears in docs/getting-started/installation.md line 10.
| After enabling the module, visit **Administration > Configuration > System > | |
| RL Settings** (`/admin/config/system/rl`) to configure the module. | |
| After enabling the module, visit **Administration > Configuration > Web services > | |
| RL: A/B Testing** (`/admin/config/services/reinforcement-learning`) to configure the module. |
| drush en rl | ||
| ``` | ||
|
|
||
| After enabling, visit `/admin/config/system/rl` to review the default settings, |
There was a problem hiding this comment.
Wrong settings path (same issue as in configuration.md): the real route is /admin/config/services/reinforcement-learning; /admin/config/system/rl returns a 404.
| After enabling, visit `/admin/config/system/rl` to review the default settings, | |
| After enabling, visit `/admin/config/services/reinforcement-learning` to review the default settings, |
| // Check if an experiment exists. | ||
| $exists = $registry->exists('my-experiment'); | ||
|
|
||
| // Remove an experiment and all its data. | ||
| $registry->delete('my-experiment'); |
There was a problem hiding this comment.
Undefined methods: src/Registry/ExperimentRegistryInterface.php declares only register(), isRegistered(), getOwner(), getAll(), and getExperimentName(). There is no exists() and no delete(); code copied from this example fatals with "Call to undefined method". The existence check is isRegistered(), and experiment deletion has no registry API (the drush rl:experiment:delete command deletes the rl_arm_data/rl_arm_snapshots rows directly).
| // Check if an experiment exists. | |
| $exists = $registry->exists('my-experiment'); | |
| // Remove an experiment and all its data. | |
| $registry->delete('my-experiment'); | |
| // Check if an experiment exists. | |
| $exists = $registry->isRegistered('my-experiment'); | |
| // Removing an experiment and all its data has no registry API; | |
| // use `drush rl:experiment:delete my-experiment` instead. |
| ``` | ||
|
|
||
| All three sections are optional. Invalid or unregistered entries are | ||
| dropped silently so one bad event does not poison the rest of the |
There was a problem hiding this comment.
Wrong error contract: rl.php does not drop invalid batch entries silently. Rejected entries are reported in an errors array with machine-readable reasons (unknown_experiment, invalid_arm_id, missing_arms, ...), and when every entry of a non-empty batch is rejected the endpoint returns HTTP 422 with {"ok": false} (rl.php lines 111-122). The error table below (400/500 only) omits the 422. A client written to this doc, expecting 200 plus ok: true in all cases, will misclassify the 422 as a transport/server failure and may retry endlessly; js/rl.js itself special-cases exactly this 422 (line 175).
| After enabling, visit `/admin/config/system/rl` to review the default settings, | ||
| then see [Configuration](configuration.md) for next steps. | ||
|
|
||
| ## Plotly.js library (required for charts) |
There was a problem hiding this comment.
Section is dead weight: nothing in the module loads from web/libraries/plotly.js-dist-min/. rl.libraries.yml defines the plotly library as the bundled js/vendor/plotly.min.js (committed to the repo, v2.35.2) and ReportsController attaches rl/plotly; there is no hook_library_info_alter and no reference to the Asset Packagist path anywhere. Charts work with zero extra installation, so these four composer steps send every installer through pointless setup, and "required for charts" is false. Either drop the section or make the module actually consume the external library. (README.md carries the same stale instructions.)
| # Source: https://git.drupalcode.org/project/gitlab_templates/ | ||
| ################ | ||
|
|
||
| include: |
There was a problem hiding this comment.
Opts into the full DrupalCI matrix, not just Pages: the pages/mkdocs job does live inside include.drupalci.main.yml (and gitlab_templates offers no pages-only include), but SKIP_COMPOSER_LINT, SKIP_PHPCS, SKIP_PHPSTAN, SKIP_STYLELINT, SKIP_ESLINT, and SKIP_CSPELL all default to '0', so every drupalcode push/MR now also runs those jobs with the templates' own configs. This repo has no cspell wordlist and no phpstan config, and standardizes lint on docker compose plus GitHub Actions, so the first pipeline will show unrelated red/orange jobs and any pages-job failure gets lost in the noise. (PHPUnit is safe: that job is gated by a tests-exist rule and this repo has only scripts/e2e/.) Consider setting the documented SKIP_* or allow-failure variables in this same commit, or note explicitly that adopting full DrupalCI is intentional and was verified against a test pipeline. Also worth knowing: the pages job deploys only on pushes to drupalcode's default branch that touch docs/** or mkdocs.yml; nothing deploys from GitHub, MRs, or feature branches.
| site_url: 'https://project.pages.drupalcode.org/rl/' | ||
| repo_url: https://git.drupalcode.org/project/rl | ||
| repo_name: drupal/rl | ||
| edit_uri: /-/edit/1.x/docs/ |
There was a problem hiding this comment.
Broken edit links (currently inert): with a leading slash, MkDocs resolves edit_uri against the host root and drops the project path: urljoin('https://git.drupalcode.org/project/rl/', '/-/edit/1.x/docs/index.md') yields https://git.drupalcode.org/-/edit/1.x/docs/index.md, a 404 (verified against the mkdocs 1.6 _set_edit_url source). Nothing surfaces it today because Material only renders the edit button when the content.action.edit theme feature is enabled, which it is not; the moment someone enables it, every edit link on the site 404s. Drop the leading slash, and add content.action.edit to theme.features if the button is wanted (or delete edit_uri instead of shipping dead-but-broken config).
| edit_uri: /-/edit/1.x/docs/ | |
| edit_uri: '-/edit/1.x/docs/' |
| @@ -0,0 +1,33 @@ | |||
| site_name: 'RL: A/B & Multivariate Testing - Documentation' | |||
| site_url: 'https://project.pages.drupalcode.org/rl/' | |||
There was a problem hiding this comment.
Stray file will be published (anchored here because the file predates this PR): docs/rl_project_desc.html is a tracked drupal.org project-description HTML snippet containing a dxpr.com/?ref=50701 referral link. MkDocs copies non-markdown files in the docs dir verbatim into the built site, and no exclude_docs is configured, so the Pages build this PR enables will publish it at https://project.pages.drupalcode.org/rl/rl_project_desc.html as an unstyled orphan page. Add exclude_docs: rl_project_desc.html or move the file out of docs/.
| services: | ||
| my_module.experiment_decorator: | ||
| class: Drupal\my_module\Decorator\MyExperimentDecorator | ||
| arguments: ['@entity_type.manager'] |
There was a problem hiding this comment.
Example contradicts itself: this registration passes arguments: ['@entity_type.manager'] to a class that has no constructor and instead calls \Drupal::entityTypeManager() statically (line 38). PHP silently ignores constructor args on a constructor-less class, so the injected service is dead weight, and the canonical example teaches the static-call pattern the module's own decorators avoid (modules/rl_menu_link injects via constructor). Separately, ReportsController calls decorateArm() once per arm, so the per-arm load($arm_id) in the example is an N+1 entity-load pattern on the reports page; the bundled VariantExperimentDecoratorBase shows the cheaper shape (resolve once, per-request cache). Suggest adding a constructor that stores EntityTypeManagerInterface and memoizing loads.
| @@ -0,0 +1,74 @@ | |||
| # RL: A/B & Multivariate Testing for Drupal | |||
There was a problem hiding this comment.
Duplication without a canonical source: docs/** copies most of the 600-line README (install steps, Plotly section, Nginx block, API references, drush tables) and the README gains no pointer to the new docs site. Drift is already visible inside this PR: README's rl.php smoke test uses action=turns against an unregistered experiment (silently dropped, so it proves nothing) while installation.md correctly uses action=ping; README's PHP example calls getThompsonScores('my-experiment') single-arg while docs/api/php.md shows the three-argument form. Suggest trimming README's deep-reference sections to links into this site, or at minimum adding a prominent docs-site link, and naming which copy is canonical.
jjroelofs
left a comment
There was a problem hiding this comment.
Not LGTM yet. All eight findings from the first review are substantively fixed, the current head passes every GitHub check, and CI=true mkdocs build --strict succeeds. However, the integrations guide added after that review introduces two new copy/paste or factual blockers below; I also noted one remaining UI-label mismatch. The previously posted threads about the full DrupalCI matrix, inert broken edit_uri, and the published orphan rl_project_desc.html also remain applicable.
| function rl_menu_link_preprocess_menu(array &$variables) { | ||
| foreach ($variables['items'] as &$item) { | ||
| $plugin_id = $item['original_link']->getPluginId(); | ||
| $result = $selector->selectForPluginId($plugin_id); |
There was a problem hiding this comment.
[P1] Make the menu hook example executable and recursive. This snippet never initializes $selector, so copied code fails with an undefined-variable error. It also loops only over the top-level items even though the text says it walks the menu tree; nested links are skipped. The actual module obtains rl_menu_link.variant_selector and passes it through the recursive _rl_menu_link_process_items() helper. Please include the service lookup and recursion (or clearly mark the omitted pieces) so the worked example demonstrates the behavior it claims.
| The pattern is fully server-side: the sort plugin calls | ||
| `getThompsonScores()` with the content IDs from the View results as arm IDs, | ||
| then reorders the rows before rendering. Turns are batched via | ||
| `Drupal.rl.turn()` for all visible items; rewards fire when a visitor clicks |
There was a problem hiding this comment.
[P1] Describe rl_sorting's actual transport. The current rl_sorting 1.x implementation does not call Drupal.rl.turn(): js/rl-sorting-tracking.js collects visible arms for 100 ms, then sends a legacy action=turns FormData payload with navigator.sendBeacon(); rewards are also direct legacy form posts. This sentence contradicts this PR's own JavaScript API page, which correctly documents that behavior. Please keep the guide consistent with the production consumer.
| ## Module settings | ||
|
|
||
| After enabling the module, visit **Administration > Configuration > | ||
| Web services > Reinforcement Learning** |
There was a problem hiding this comment.
[P2] Use the menu item's actual label. The route is fixed, but rl.links.menu.yml titles this entry RL: A/B Testing, not Reinforcement Learning. Users navigating through the UI will not find the breadcrumb as written; change the final segment to the registered menu title.
|
Follow-up review at head 60e5808 The content fixes in af16ecc land well: the config keys, settings path, registry example ( Still blocking merge (unchanged since the first pass):
All five are small, mechanical changes; with them in, this looks mergeable. |
jjroelofs
left a comment
There was a problem hiding this comment.
Still not LGTM. CI and a strict MkDocs build pass, but the latest commit only updates the project-description FAQ/headings and leaves the previously reported menu-hook example, rl_sorting transport claim, and settings label unchanged. The modified project description also repeats the unsupported Drupal Routes API fallback; see the new inline comment. The earlier MkDocs/GitLab configuration concerns remain unresolved as well.
| @@ -74,7 +74,7 @@ <h4>Verify rl.php access</h4> | |||
|
|
|||
| <p>If server policies prevent direct access, use the Drupal Routes API instead.</p> | |||
There was a problem hiding this comment.
[P1] Remove the nonexistent route fallback. The module defines no tracking/API route in rl.routing.yml; its routes are admin settings and reports only. The installation page was corrected after the first review, but this project description still tells blocked clients to use an API that the module does not provide. State that direct access is required, or document a concrete custom proxy implementation instead.
| When some entries are rejected (invalid IDs, unregistered experiments, | ||
| malformed entries), the response includes an `errors` array: | ||
|
|
||
| ```json | ||
| { | ||
| "ok": false, | ||
| "errors": [ | ||
| {"kind": "decide", "id": "unknown_exp", "reason": "unknown_experiment"}, | ||
| {"kind": "turn", "id": "bad!", "reason": "invalid_id"} | ||
| ] | ||
| } |
There was a problem hiding this comment.
Tiny carry-over: this section got the pre-fix wording, so the example pairs "ok": false with "when some entries are rejected", while rl.php sets ok: false only when every entry was rejected (a partial success is 200 with ok: true plus decisions and errors). docs/api/http.md now has the corrected version; this brings the README in line with it.
| When some entries are rejected (invalid IDs, unregistered experiments, | |
| malformed entries), the response includes an `errors` array: | |
| ```json | |
| { | |
| "ok": false, | |
| "errors": [ | |
| {"kind": "decide", "id": "unknown_exp", "reason": "unknown_experiment"}, | |
| {"kind": "turn", "id": "bad!", "reason": "invalid_id"} | |
| ] | |
| } | |
| When some entries are rejected (invalid IDs, unregistered experiments, | |
| malformed entries), the response includes an `errors` array alongside | |
| any successful results. A partial success returns HTTP 200 with | |
| `ok: true`: | |
| ```json | |
| { | |
| "ok": true, | |
| "decisions": { | |
| "hero_cta": {"armId": "v1"} | |
| }, | |
| "errors": [ | |
| {"kind": "decide", "id": "unknown_exp", "reason": "unknown_experiment"}, | |
| {"kind": "turn", "id": "bad!", "reason": "invalid_id"} | |
| ] | |
| } | |
| ``` |
|
Re-review at head 79be026: all five blockers verified fixed.
One trivial carry-over, noted inline: the README's new batch-errors section received the pre-fix example ( |
jjroelofs
left a comment
There was a problem hiding this comment.
Re-reviewed at 69946ec (two commits since the last "still not LGTM").
Fixed since the last round, threads can be resolved
.gitlab-ci.yml: theSKIP_*variables now match thedrupal-module-docstemplate, so only the Pages job runs.mkdocs.yml:edit_uriwithout the leading slash,rl_project_desc.htmlexcluded.docs/guides/decorators.md: example now has the constructor that itsarguments:line needs (both decorator threads).- README: obsolete Plotly install section removed, route-fallback sentence corrected, batch error contract documented.
docs/api/http.md: partial-success semantics now matchrl.php(200 withok: true, 422 only when everything is rejected).docs/guides/faq.md: "locks the variant" claim removed.
CI=true mkdocs build --strict passes on the head, no em-dashes anywhere in docs/, README.md or mkdocs.yml, and the docs FAQ uses ## headings as the skill requires.
Still open, all carried over from the previous review
docs/guides/integrations.md:196: menu hook example uses an undefined$selectorand does not recurse.docs/guides/integrations.md:302: rl_sorting transport claim is wrong; it beacons directly torl.php, not viaDrupal.rl.turn().README.md:421: the partial-rejection example still shows"ok": false; the README andhttp.mdnow disagree.docs/getting-started/configuration.md:6(and installation.md): menu label is "RL: A/B Testing".
New
5. docs/rl_project_desc.html is stale: it equals 1.x commit 44bad48, but 1.x is already at 45b9c21. Rebase onto 1.x so the file drops out of the diff.
6. No GitHub check run exists for 69946ec; the last green run is on 79be026. Worth a re-push or manual trigger so the merge commit has a status.
The README-duplication thread from the first round is still applicable but not blocking; the README now links to the docs site, which is the minimum.
Not LGTM yet because of items 1 to 3; they are all small edits in two files.
| function rl_menu_link_preprocess_menu(array &$variables) { | ||
| foreach ($variables['items'] as &$item) { | ||
| $plugin_id = $item['original_link']->getPluginId(); | ||
| $result = $selector->selectForPluginId($plugin_id); |
There was a problem hiding this comment.
Still broken (carried over from the previous review). $selector is never defined in this snippet, and the loop only touches the top level of $variables['items'], so a copy of this fails with an undefined-variable error and ignores every child link. The shipped rl_menu_link_preprocess_menu() in modules/rl_menu_link/rl_menu_link.module delegates to a recursive _rl_menu_link_process_items() helper and attaches cache tags plus the rl_menu_link/tracking library. Either show that shape (get the selector with \Drupal::service('rl_menu_link.variant_selector'), recurse into $item['below'], add the cache tags) or replace the code with a prose description and a link to the real file.
|
|
||
| The pattern is fully server-side: the sort plugin calls | ||
| `getThompsonScores()` with the content IDs from the View results as arm IDs, | ||
| then reorders the rows before rendering. Turns are batched via |
There was a problem hiding this comment.
Still wrong (carried over). rl_sorting does not call Drupal.rl.turn(). Its js/rl-sorting-tracking.js uses an IntersectionObserver to collect visible entity IDs, then posts action=turns with a comma-separated arm_ids list via navigator.sendBeacon() straight to the rl.php URL that rl_sorting.module publishes in drupalSettings (rlEndpointUrl). Rewards are a separate action=reward beacon on link click, de-duplicated per page load with sessionStorage. Describe that transport, or drop the sentence; "fully server-side" is also inaccurate given the client-side tracking.
| ## Module settings | ||
|
|
||
| After enabling the module, visit **Administration > Configuration > | ||
| Web services > Reinforcement Learning** |
There was a problem hiding this comment.
Still open (P2 from the previous review). The path is right, but rl.links.menu.yml titles this entry RL: A/B Testing, not "Reinforcement Learning". A reader looking for the label in the admin menu will not find it. Same wording appears in installation.md.
|
|
||
| ```json | ||
| { | ||
| "ok": false, |
There was a problem hiding this comment.
Example contradicts the sentence under it and the code. rl.php line 118 sets ok to $status === 200, and a partial success is 200, so this "when some entries are rejected" example must show "ok": true with a decisions object next to errors, exactly as docs/api/http.md now does after the last commit. "ok": false only ever appears with the 422 all-rejected case. The getThompsonScores(..., NULL, ['variant-a', 'variant-b']) call above matches the real signature, no issue there.
| <p>RL is part of the <a href="https://dxpr.com/c/marketing-cms?ref=50701">DXPR marketing CMS</a> stack and ships in <a href="https://www.drupal.org/project/dxpr_cms">DXPR CMS</a>.</p> | ||
|
|
||
| <h3>What you can A/B test with RL</h3> | ||
| <blockquote>A/B and multivariate testing framework for Drupal where every visitor click is treated as human feedback (RLHF-style). Each page view is a trial, each conversion is a reward, and the algorithm continuously shifts traffic to whichever variant is winning. No fixed test horizons. No manual winner picking. No third-party SaaS.</blockquote> |
There was a problem hiding this comment.
This file is stale relative to 1.x and should leave the PR. The head version is byte-identical to 1.x commit 44bad48, but 1.x has since moved to 45b9c21, which replaced this blockquote with a bold intro paragraph and dropped the documentation note-tip box. Since the last commit's intent was "match 1.x", the clean way is to rebase onto current 1.x so docs/rl_project_desc.html disappears from the diff entirely; the docs-site links in the box only resolve after this PR publishes anyway. Line 81 ("use the Drupal Routes API instead") is a pre-existing 1.x problem now, so I would fix it in a follow-up rather than here.
|
Pushed Fixed:
Re: desc.html docs links: The |
jjroelofs
left a comment
There was a problem hiding this comment.
Re-reviewed at 4f93643.
Fixed, threads can be resolved
- Menu hook example is now recursive and resolves the selector service.
- rl_sorting transport is described accurately (IntersectionObserver plus
sendBeacon()torl.php). - Configuration label matches
rl.links.menu.yml. - README partial-rejection example now says
ok: truewithdecisionsanderrors. - Project description: intro paragraph and the "Routes API" sentence now align with 1.x.
Still blocking
docs/rl_project_desc.htmlconflicts with 1.x:git merge-treereports a content conflict in thenote-tipregion near line 134, and the branch is two commits behind. Needs a rebase before this can merge (inline).README.md:423: the new example uses"hero_cta": "v1"; the real shape is{"armId": "v1"}(inline).
Minor
3. docs/guides/integrations.md:313: "fully server-side" now contradicts the corrected tracking description (inline).
4. No GitHub Actions run exists for 69946ec or 4f93643; the workflow is on: [pull_request] and the last run is on 79be026. A rebase push will trigger it, which also resolves item 1.
Strict MkDocs build still passes on this head. Not LGTM until the rebase lands and the README shape is fixed; both are small.
| { | ||
| "ok": true, | ||
| "decisions": { | ||
| "hero_cta": "v1" |
There was a problem hiding this comment.
Wrong decision shape. rl.php returns {"armId": "<arm>"} objects (line 284, and the success example twenty lines above this one shows "hero_cta": {"armId": "v1"}). This partial-success example has "hero_cta": "v1" as a bare string. Match it to docs/api/http.md line 73.
| It provides a Views sort plugin that reorders view results by Thompson | ||
| Sampling score, turning any View into an engagement-optimised ranking. | ||
|
|
||
| The pattern is fully server-side: the sort plugin calls |
There was a problem hiding this comment.
Sentence now contradicts the paragraph. The new text correctly describes client-side IntersectionObserver plus sendBeacon() tracking, so "fully server-side" is no longer true. Suggest "The sort is server-side: ..." and keep the tracking description as is.
| </ul> | ||
|
|
||
| <h3>API</h3> | ||
| <div class="note-tip"> |
There was a problem hiding this comment.
Merge conflict with 1.x. git merge-tree 1.x feat/mkdocs-pages conflicts in this file around this note-tip block (1.x 45b9c21 reworked the same region). The branch is still two commits behind 1.x, so GitHub will refuse the merge. Rebase onto current 1.x and resolve; if the docs-link box should survive, re-add it on top of 1.x's version.
Add MkDocs Material documentation structured from the README content, split into logical sections: getting started, API reference (PHP, JS, HTTP), guides (Drush, decorators, cache, AI assistant), and development. Includes .gitlab-ci.yml with standard DrupalCI templates to enable automatic publishing to project.pages.drupalcode.org/rl/. Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
Add doc comments and fix concat operator spacing in the decorator code example so phpcs passes on documentation markdown files. Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
…as PHP The opening PHP tag caused phpcs to treat the entire markdown file as PHP, including the YAML services example further down. Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
…ages - Add FAQ page with common questions from the README - Fix Configuration page: add actual module settings content, report viewing section, and image placeholders for settings form and reports - Expand PHP API page with experiment registry examples, remove duplicated cache content (now links to canonical cache guide) - Expand cache guide with client-side alternative section - Expand contributing page with branch conventions, architecture overview, and test instructions - Add image/diagram placeholders throughout for later insertion - Add cross-links between installation and configuration pages Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
- Fix settings route: /admin/config/services/reinforcement-learning (was /admin/config/system/rl which returns 404) - Fix Drush config examples: use valid settings debug_mode and event_log_max_rows (was nonexistent exploration_rate) - Fix registry API: use isRegistered() not exists(), use purgeExperiment() not delete() (matching actual interfaces) - Remove obsolete Plotly.js installation section (library is bundled in js/vendor/plotly.min.js) - Remove false "Drupal Routes API" fallback claim (no such route exists); document custom module proxy as the alternative - Document batch error responses: add errors array format and HTTP 422 status code to error table - Fix decorator example: add constructor injection for entity_type.manager to match the services.yml definition - Fix FAQ: Thompson Sampling preserves exploration, losing arms get less traffic but never zero; they stop only when removed Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
Add three screenshots from a live RL installation: - Experiment list with impressions, conversions, and variant counts - 3D Posterior Landscape chart (hero image on home page) - 2D Line Chart showing conversion scores over time Replace TODO image placeholders in index.md and configuration.md with the actual screenshots. Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
Place the hero screenshot immediately after the intro paragraph for maximum visual impact on first visit. Claude-Session: https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR
New guide walks through the full anatomy of a consumer module using rl_page_title and rl_menu_link as worked examples: experiment entity, variant selector, report decorator, hook integration, and tracking JS. Covers rl_sorting (Views) and DXPR Builder integration patterns. Enriches the home page "What you can A/B test" section with detail on how each integration works. Cross-references from configuration and contributing docs.
1. Skip uncurated DrupalCI lint jobs (SKIP_* variables in .gitlab-ci.yml) 2. Fix edit_uri leading slash in mkdocs.yml (broken edit links) 3. Exclude rl_project_desc.html from Pages build (orphan page with referral link) 4. README drift: remove obsolete Plotly install, fix smoke test to use action=ping, fix getThompsonScores to three-arg form, add batch error/422 docs, fix decorator to use constructor injection, add docs site link, fix "Drupal Routes API" claim 5. Wording: remove nonexistent "locks" feature from faq.md and project desc, fix http.md ok:false example to show partial success correctly
- Fix menu hook example: initialise $selector, add recursion via _rl_menu_link_process_items() to match the shipped implementation - Fix rl_sorting transport: uses sendBeacon() directly to rl.php, not Drupal.rl.turn() - Fix README batch error example: partial success is ok:true with both decisions and errors; 422 with ok:false only when all rejected - Fix configuration.md menu label: "RL: A/B Testing" per rl.links.menu.yml - Update desc.html to latest 1.x and re-add docs callout box - Remove nonexistent "Drupal Routes API" fallback claim from desc.html
Match rl.php response format: decisions use {"armId": "v1"} objects,
not bare strings.
…ording Re-add documentation links callout box to desc.html after rebase dropped it. Fix "fully server-side" in integrations.md to correctly distinguish server-side decisions from client-side tracking.
3da9234 to
aa60486
Compare
|
Addressed all remaining items:
The force push was necessary due to the rebase. |
jjroelofs
left a comment
There was a problem hiding this comment.
LGTM at aa60486.
Verified on this head:
- Rebased onto current 1.x (zero commits behind),
git merge-treeis clean, GitHub reports mergeable. - All GitHub checks green (
drupal-lint,drupal-check,drush-e2e,changes; eslint skipped by design). CI=true mkdocs build --strictpasses; no em-dashes indocs/,README.mdormkdocs.yml.- README partial-success example now uses
{"armId": "v1"}; rl_sorting paragraph separates the server-side decision from the client-side tracking. - The four
docs/rl-experiment*.{jpg,png}files in the diff against my last review come from 1.x commits44bad48/45b9c21, not from this branch; relative to 1.x the PR adds only the three referenceddocs/images/*.jpg. - The four docs-site links in the project description (
/,/api/php/,/getting-started/installation/,/guides/integrations/) all map to pages in themkdocs.ymlnav.
Two non-blocking notes for after merge:
- Push 1.x to the
doremote first and confirmproject.pages.drupalcode.org/rl/serves before deploying the project description to drupal.org, otherwise the callout links 404 for a while. - The README/docs duplication thread from the first round stays open as a follow-up; the README now links to the site, which is the minimum.
GitHub blocks approving your own PR, so this is a comment review.
Summary
.gitlab-ci.ymlwith standard DrupalCI templates to enable automatic publishing toproject.pages.drupalcode.org/rl/mkdocs.ymlconfiguration for Material themeDocumentation pages
Drupal.rl), HTTP API (rl.php)Test plan
project.pages.drupalcode.org/rl/https://claude.ai/code/session_01Cbtzf2mpa3xQKCSkFzJuiR