Automattic / Automattic/syndication

Break up the 3.x rewrite in PR #197 into reviewable slices

Open
#218 0 comments 0 reactions 0 assignees View on GitHub
type: maintenance
Dominant language
PHP
Stars
107
Forks
44
Avg merge
18m
Merged PRs (30d)
19

Description

## Why this exists

[PR #197](https://github.com/Automattic/syndication/pull/197) grew to 22 commits and 60 files (+11,335 / −1,706) covering at least seven separable concerns. It is being parked rather than merged. This issue captures the diagnosis while it is fresh so the work can be picked up in slices, and so the branch is not simply rediscovered cold in six months.

The branch itself is preserved at `refactor/cli-test-strategy`. Nothing here needs to be reconstructed from scratch — most of it is sound work that was merely bundled past the point of reviewability.

## What went wrong, and why it matters for the restart

Until [PR #217](https://github.com/Automattic/syndication/pull/217), the integration workflow only ran for pull requests into `develop` and `main`, so the entire 3.x rewrite was assembled without a single integration run. Its green ticks meant linting and unit tests alone. The moment integration tests were switched on for 3.x, five independent breakages surfaced one behind the other:

| Problem | Introduced by |
| --- | --- |
| `Walker_CategoryDropdownMultiple` deleted as "unused" while still required and instantiated by the XML client — fatal on activation | `15ceb12` |
| `SyndicationNotifier` instantiated on every request; the class has never existed anywhere in the repository's history | `3c8c541` |
| `Container::instance()` called from six sites, including the base class every integration test extends; the method was never written | `3c8c541` |
| `Integration/TestCase.php` never required, killing PHPUnit in the bootstrap | `e2b2a52` |
| `Syndication_Mock_Client.php` require replaced by the new CLI requires instead of added alongside | `e2b2a52` |

All five are fixed on the branch (`8eb4911`, `f87e558`, `35880f0`, `45f7b5c`) and are small and orthogonal, so they can be cherry-picked onto whichever slice lands first.

## What is still broken on the branch

**Integration tests: 31 of 75 fail.** Every one of them tests production code that the branch's own commits deleted or replaced, and which was never retired alongside it:

- `tests/Integration/Application/HookRegistrationTest.php` and `BootstrapperIntegrationTest.php` assert on `$GLOBALS['syndication_bootstrapper']` and `Application\Bootstrapper`, which `PluginBootstrapper` superseded. `Application\Bootstrapper` is now referenced by nothing but its own tests.
- `tests/Integration/EncryptionTest.php` and `EncryptorOpenSSLTest.php` use the legacy encryption classes. Those four files still sit in `includes/` but nothing loads them any more; `Infrastructure\Encryption\OpenSSLEncryptor` replaced them.
- `tests/Integration/PullContentTest.php` calls `WP_Push_Syndication_Server::pull_content()`, removed in `aed74aa`.

For comparison, `3.x` itself runs 20 integration tests green. The branch's own 22 CLI integration tests also pass once the bootstrap is fixed.

**Two gaps CI cannot see**, because neither path is exercised by WP-CLI:

- `PluginBootstrapper::register_admin_services()` resolves six `Infrastructure\Admin\*` classes — `SiteListTable`, `SettingsPage`, `SiteMetaboxes`, `PostSyndicationMetabox`, `AdminAssets`, `AdminMessages` — none of which exist. Any wp-admin request would fatal. The PR assumes an admin extraction it never performs.
- `CredentialTestingService` and `class-wp-push-syndication-server.php` both instantiate `Infrastructure\Transport\REST\WordPressRestTransport`, which was never written. `WordPressComTransport` is not a substitute: it takes an OAuth token and blog ID, where these callers have a username and password. Testing WP_REST_API credentials therefore fatals.

## Proposed slices

Ordered by dependency. Each should be independently reviewable and independently green.

1. **DI container** — `Container` plus the `instance()` accessor its callers assume.
2. **Plugin bootstrap** — `PluginBootstrapper` replacing `Application\Bootstrapper`, deleting that class and its tests in the same pull request.
3. **Admin extraction** — the six `Infrastructure\Admin\*` classes pulled out of `class-wp-push-syndication-server.php`. Blocks anything that touches wp-admin.
4. **Encryption** — migrate to `Infrastructure\Encryption\OpenSSLEncryptor`, deleting the four legacy classes and their tests together.
5. **Logging consolidation** — per-sync entries and the log viewers.
6. **Cron and hook handlers** — `HookRegistrar` and the async push/pull handlers.
7. **Transports** — including the missing `WordPressRestTransport`, or removal of the callers that expect it.
8. **CLI commands** — last, since they depend on 1 to 3.

Separately, the notification settings fields on the settings screen have never had a sender. There is no `wp_mail` call anywhere on `3.x` or `develop`. Either wire them up or remove the fields.

## Two rules that would have caught all of this

- A pull request may not delete production code without retiring its tests in the same commit.
- A pull request may not reference a class it does not add.

Both failures in the table above are instances of one or the other.

## Related

- [PR #197](https://github.com/Automattic/syndication/pull/197) — the branch being broken up
- [PR #217](https://github.com/Automattic/syndication/pull/217) — makes integration tests run for pull requests into `3.x`; merge before starting any slice, or the next attempt repeats this
- [PR #214](https://github.com/Automattic/syndication/pull/214) — changed-lines PHPCS gate for `develop`; worth cherry-picking to `3.x`, which carries 513 errors and 106 warnings across 18 files
- Dependabot reads `.github/dependabot.yml` from the default branch only, which is why `3.x` fell a major version behind on three actions. Keeping it current needs `target-branch: "3.x"` entries added on `develop`.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with the preserved refactor/cli-test-strategy branch and the proposed dependency order, beginning with Container and its instance() callers. Read the named integration tests and run the integration suite to confirm the current failures; a completed slice should be independently reviewable and green, with obsolete production code and tests retired together.

Written by the indexing model from the issue text.

Assessment

Tech stack
github-actions, php
Domain
backend, ci-cd, cli, testing
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.