Skip to content

refactor(sdk): OOP modernization — AbstractClient, HttpTransport, SessionCapable trait#167

Open
KaiSchwarz-cnic wants to merge 12 commits into
masterfrom
RSRMID-2790/oop-modernization-fixes
Open

refactor(sdk): OOP modernization — AbstractClient, HttpTransport, SessionCapable trait#167
KaiSchwarz-cnic wants to merge 12 commits into
masterfrom
RSRMID-2790/oop-modernization-fixes

Conversation

@KaiSchwarz-cnic
Copy link
Copy Markdown
Collaborator

@KaiSchwarz-cnic KaiSchwarz-cnic commented Jun 1, 2026

Jira: RSRMID-2790

Summary

A series of OOP modernization commits that restructure the client hierarchy, extract shared transport logic, fix an LSP violation in IBS\\SessionClient, and clean up several smaller code-quality issues.

Architecture changes

  • AbstractClient introduced (CNIC namespace): consolidates all registrar-agnostic logic (config loading, cURL transport setup, credentials, OTE/LIVE switching, IDN conversion, debug mode, proxy/referer, user agent, settings/isOTE accessors). Three abstract methods define the contract every concrete client must satisfy: request(), newSocketConfig(), setDefaultLogger().
  • HttpTransport introduced (CNIC namespace): owns the cURL handle lifecycle and exposes a single post() method. The $chandle property has been removed from the client layer entirely; close() now delegates to the transport. Class is final (not public API, not extended).
  • CNR\\Client now extends AbstractClient and retains only CNR-specific logic: request() with CNR command formatting, requestNextResponsePage(), and requestAllResponsePages().
  • IBS\\Client now extends AbstractClient directly instead of CNR\\Client, correctly reflecting that IBS shares the common foundation but not the CNR-specific layer.

Session handling

  • SessionCapable trait extracted into CNIC\\CNR: holds the real session implementations (login, logout, saveSession, reuseSession).
  • CNR\\SessionClient uses the trait.
  • IBS\\SessionClient no longer carries misleading stubs that unconditionally threw "Method not supported" — it simply does not include the trait.
  • MONIKER\\SessionClient is unaffected (also does not support sessions).

⚠️ Breaking change: callers that caught \\Exception("Method not supported") from IBS\\SessionClient session methods will now receive a fatal Error ("Call to undefined method"). Guard with instanceof CNR\\SessionClient before calling session methods when supporting multiple registrar types.

Smaller improvements

  • $settings and $isOTE made protected with public getSettings(): array and isOTE(): bool accessors (previously public, allowing unintended external mutation).
  • ClientFactory::getClient() $logger parameter widened from ?CNR\\Logger to ?LoggerInterface so any logger implementation can be passed without a TypeError.
  • Dead IBS\\Client::getPOSTData() override removed — AbstractClient already delegates to $this->socketConfig->getPOSTData() via polymorphism; the override was byte-for-byte identical to the base.
  • SessionCapable::saveSession() and reuseSession() given : static return type declarations to satisfy the fluent contract.
  • curl_close() calls removed — deprecated since PHP 8.5; CurlHandle is freed automatically by the GC.
  • Registrar enum introduced in ClientFactory; switch replaced with match.
  • Column and Record value objects updated to constructor promotion + readonly.
  • $logger Psalm PropertyNotSetInConstructor suppression added (always set via abstract setDefaultLogger() in AbstractClient::__construct() — Psalm cannot trace through abstract method dispatch).

Release automation

  • .releaserc.json updated: the semantic-release-replace-plugin file target and the @semantic-release/git assets were changed from src/CNR/Client.php to src/AbstractClient.php, where getVersion() now lives after the refactor.

Eliminates the ~30-line duplicate cURL block between CNR\Client and
IBS\Client. The new protected executeCurl() handles handle init, option
setup, execution, and error detection, returning [raw, error|null].

Each request() method retains only its brand-specific logic — command
preparation, URL assembly, extra cURL options — and creates its own
Response subtype directly, keeping the type hierarchy clean without
factory indirection.

Also fixes the latent bug in IBS\Client where debug logging passed
secured=false, which would have exposed passwords in debug output.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@KaiSchwarz-cnic KaiSchwarz-cnic self-assigned this Jun 1, 2026
KaiSchwarz-cnic and others added 9 commits June 1, 2026 11:33
Uses PHP 8.0 match expression with comma-separated arms and throw
expressions. Consolidates three separate setCustomLogger() calls into
one ternary based on instanceof IBSSessionClient, which naturally
covers both IBS and MONIKER (the latter extends the former).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… and Record

Applies PHP 8.1 constructor promotion to eliminate separate property
declarations and constructor assignments. All properties are set once
and never mutated, so readonly is added throughout.

Column: $key and $data promoted to private readonly; $length remains
a regular public readonly property since it is computed (count($data))
rather than assigned directly from a parameter.

Record: $data promoted to private readonly; constructor body is now
empty. The duplicate @var docblock is removed — the type is already
declared on the promoted parameter.

IBS\Column and IBS\Record are empty subclasses and require no changes.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…entFactory

Adds CNIC\Registrar (backed string enum) covering all recognised
registrar identifiers including the two EOL cases. ClientFactory now
resolves the input via Registrar::tryFrom() and matches on enum cases,
replacing the raw string literals. Unrecognised input resolves to null,
which the match arm converts to the existing "not supported" exception.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Both properties were public, allowing external code to read and mutate
internal configuration state directly. They are now protected with
public getSettings(): array and isOTE(): bool accessors.

Three test locations that mutated \$settings are updated:
- testGetUrl: reads via getSettings() instead of direct access
- testSetRemoteIpAddressSetThrows / Set: use a fresh SessionClient()
  (no config = no ipfilter key) instead of unsetting from shared state,
  removing a hidden ordering dependency between tests
- testRequestCurlExecFail2: chains ->useOTESystem()->setURL("bad-url")
  rather than writing to settings; testRequestFlattenCommand simply
  calls useOTESystem() to restore the correct URL from settings

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t LSP violation

IBS\SessionClient had four methods (login, logout, saveSession,
reuseSession) that unconditionally threw "Method not supported" — the
class was named SessionClient but incapable of session communication.

Fix: extract the real session implementations into a CNIC\CNR\SessionCapable
trait. CNR\SessionClient uses the trait; IBS\SessionClient does not,
so it no longer carries misleading stubs. MONIKER\SessionClient (which
extends IBS) is unaffected — it also does not support sessions.

The trait is the extension point for any future registrar that does
support sessions: include it and the capability is available.

Test adjusted: testSaveReuseSession instantiates SessionClient directly
rather than via ClientFactory so PHPStan can infer the precise type.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…, #14)

AbstractClient (CNIC namespace) is the shared foundation for all
registrar API clients. It holds every property and method previously
in CNR\Client that is not CNR-specific: configuration loading,
transport setup, debug mode, user agent, proxy/referer, credentials,
IDN conversion, OTE/LIVE switching, settings/isOTE accessors, and the
executeCurl() bridge. Three abstract methods define the contract each
concrete client must satisfy: request(), newSocketConfig(), and
setDefaultLogger().

HttpTransport (CNIC namespace) owns the cURL handle lifecycle and
exposes a single post() method. The $chandle property is removed from
the client layer entirely; close() now delegates to the transport.

CNR\Client extends AbstractClient and retains only what is CNR-specific:
request() with CNR command formatting, requestNextResponsePage(), and
requestAllResponsePages().

IBS\Client now extends AbstractClient directly instead of CNR\Client,
correctly reflecting that IBS shares the common foundation but not the
CNR-specific layer. The redundant constructor override is eliminated
since AbstractClient already calls newSocketConfig() during construction.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tpTransport final; drop curl_close

- \$logger is always set via abstract setDefaultLogger() called in
  AbstractClient::__construct() — suppression is justified since Psalm
  cannot trace through abstract method dispatch
- HttpTransport is never extended and not public API → final
- curl_close() is deprecated since PHP 8.5; CurlHandle is freed
  automatically by the GC when set to null

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- SessionCapable::saveSession() and reuseSession(): add ': static'
  return type declarations to match the fluent contract of every other
  setter in AbstractClient (PHPStan was correct to flag the gap)
- IBS\Client::getPOSTData(): remove dead override — AbstractClient
  already delegates to \$this->socketConfig->getPOSTData(), which
  resolves to IBS\SocketConfig via polymorphism; the override was
  byte-for-byte identical to the base and added nothing
- ClientFactory::getClient(): widen \$logger parameter from ?CNR\Logger
  to ?LoggerInterface so callers can pass any logger implementation
  without a TypeError, consistent with the multi-registrar reality

BREAKING CHANGE: IBS\SessionClient no longer defines login(), logout(),
saveSession(), or reuseSession(). Previously these methods threw
\Exception("Method not supported") — callers handling the unsupported
case via try/catch will now receive a fatal "Call to undefined method"
Error instead. Guard with instanceof CNR\SessionClient before calling
session methods if your code supports multiple registrar types.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@KaiSchwarz-cnic KaiSchwarz-cnic force-pushed the RSRMID-2790/oop-modernization-fixes branch from 7b34f38 to 46eaacd Compare June 1, 2026 14:12
@KaiSchwarz-cnic KaiSchwarz-cnic changed the title refactor(client): extract shared cURL transport into executeCurl() refactor(sdk): OOP modernization — AbstractClient, HttpTransport, SessionCapable trait Jun 1, 2026
@KaiSchwarz-cnic KaiSchwarz-cnic marked this pull request as ready for review June 1, 2026 14:34
@KaiSchwarz-cnic KaiSchwarz-cnic requested a review from a team as a code owner June 1, 2026 14:34
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