magento / magento/magento-coding-standard

feat: add "No store/website resolution in constructors"

Open
#502 1 comment 3 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

proposal
Dominant language
PHP
Stars
375
Forks
165
PR merge metrics
No merged PRs in 30d

Description

`Magento2.Constructor.NoStoreResolutionInConstructor`

### Rule

Do not resolve store or website context inside a constructor. A class must not call `StoreManagerInterface` resolution methods — `getStore()`, `getStores()`, `getWebsite()`, `getWebsites()`, `getStoreId()`, `getWebsiteId()`, `getGroup()`, `getGroups()`, `getDefaultStoreView()` — from `__construct()` or from Magento's internal `_construct()` (the instantiation hook on `AbstractModel`, resource models, and collections). Inject the dependency and resolve the store lazily in the method that actually uses it.

### Reason

Magento instantiates objects through the DI container during `setup:install`, `setup:di:compile`, and `setup:static-content:deploy`, when no store/website scope exists yet. Resolving the store in a constructor at that point throws `The default website isn't defined.` and aborts the install. Worse, the `scopes` app config is memoized for the life of the process, so a single premature `getStore()` call (while the `store_website` table is still empty during schema-patch application) **caches an empty result** and breaks later, unrelated core setup patches (e.g. `Magento\InventorySales\...\InitializeWebsiteDefaultStock`) with the same error — even after the default website row has been written. The failure is also environment-dependent: it hides locally where the DB and config cache are already warm, and only surfaces on a clean CI install, making it expensive to diagnose. Keeping constructors free of scope resolution makes objects safe to instantiate in any context and removes a whole class of order-of-bootstrap bugs.

### Implementation

Enforced by the PHP_CodeSniffer sniff `Magento2.Constructor.NoStoreResolutionInConstructor` (`dev/phpcs/Magento2/Sniffs/Constructor/NoStoreResolutionInConstructorSniff.php`), registered in `phpcs.xml` and run via `composer lint`. For every `__construct`/`_construct` it walks the method body and reports (as an error) any call to a configured store-resolution method, while skipping nested closures/arrow-functions (whose bodies run on invocation, not at construction) and ignoring same-named property access that isn't a call. The forbidden-method list is a public property on the sniff, so it can be extended from the ruleset. Run locally with:

```bash
composer lint
# or just this rule:
vendor/bin/phpcs --standard=dev/phpcs/Magento2/ruleset.xml app/code/Magento2/
```

Deliberate exceptions can be suppressed inline with `// phpcs:ignore Magento2.Constructor.NoStoreResolutionInConstructor`.

Example Sniff:

```php
getTokens();

// Only inspect constructors: PHP's __construct and Magento's internal
// _construct (called at instantiation by AbstractModel/collections/etc.).
$namePtr = $phpcsFile->findNext(T_STRING, $stackPtr);
if ($namePtr === false
|| !in_array(strtolower($tokens[$namePtr]['content']), ['__construct', '_construct'], true)
) {
return;
}

// Interface/abstract method declarations have no body.
if (!isset($tokens[$stackPtr]['scope_opener'], $tokens[$stackPtr]['scope_closer'])) {
return;
}

$forbidden = array_map('strtolower', $this->forbiddenMethods);
$ptr = $tokens[$stackPtr]['scope_opener'] + 1;
$end = $tokens[$stackPtr]['scope_closer'];

while ($ptr < $end) {
$token = $tokens[$ptr];

// Skip nested function/closure/arrow-fn bodies: code there runs when
// the callback is invoked, not at construction time.
if (in_array($token['code'], [T_FUNCTION, T_CLOSURE, T_FN], true)
&& isset($token['scope_closer'])
) {
$ptr = $token['scope_closer'] + 1;
continue;
}

$isObjectAccess = $token['code'] === T_OBJECT_OPERATOR
|| $token['code'] === T_NULLSAFE_OBJECT_OPERATOR;

if ($isObjectAccess) {
$methodPtr = $phpcsFile->findNext(T_WHITESPACE, $ptr + 1, null, true);
if ($methodPtr !== false
&& $tokens[$methodPtr]['code'] === T_STRING
&& in_array(strtolower($tokens[$methodPtr]['content']), $forbidden, true)
) {
// Confirm it's a call (followed by a parenthesis), not a property.
$afterPtr = $phpcsFile->findNext(T_WHITESPACE, $methodPtr + 1, null, true);
if ($afterPtr !== false && $tokens[$afterPtr]['code'] === T_OPEN_PARENTHESIS) {
$phpcsFile->addError(
'Do not resolve store/website context in __construct(); "%s()" needs a '
. 'bootstrapped application and breaks during setup:install. '
. 'Inject the dependency and resolve the store lazily where it is used.',
$methodPtr,
'StoreResolutionInConstructor',
[$tokens[$methodPtr]['content']]
);
}
}
}

$ptr++;
}
}
}

```

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with dev/phpcs/Magento2/Sniffs/Constructor/NoStoreResolutionInConstructorSniff.php and its registration in phpcs.xml. Run composer lint, or vendor/bin/phpcs --standard=dev/phpcs/Magento2/ruleset.xml app/code/Magento2/, and verify that listed store-resolution calls in constructors are reported while nested closures and same-named property access are ignored.

Written by the indexing model from the issue text.

Assessment

Tech stack
php
Domain
tooling
Issue type
Feature
Difficulty
3/5
Estimated time
1-2 days
Activity status
Quiet
Clarity
Clearly specified
Newbie friendliness
78/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.