Refactor generation flow and upgrade docs#1
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the AmoDocsEngine into a modular service-oriented architecture, introducing dedicated classes for amoCRM integration, document generation, security, and storage. Key additions include a new quote calculation endpoint, a token-based authentication system for browser requests, and a comprehensive PHPUnit test suite. Feedback identifies several opportunities to improve robustness by replacing the error suppression operator (@) with explicit error handling and validation for filesystem operations in the document generation, logging, and storage services.
|
|
||
| $templatePath = $this->templateRegistry->path($template); | ||
| $documentDir = rtrim((string)$this->config['document_path'], '/'); | ||
| @is_dir($documentDir) || @mkdir($documentDir, (int)$this->config['dir_mode'], true); |
There was a problem hiding this comment.
Using the error control operator (@) for mkdir can hide filesystem permission issues. If the directory for documents cannot be created, the subsequent saveAs call on the template processor will fail, and the root cause might not be obvious. Explicitly checking for success and throwing an exception would make this more robust.
if (!is_dir($documentDir) && !mkdir($documentDir, (int)$this->config['dir_mode'], true) && !is_dir($documentDir)) {
throw new \RuntimeException(sprintf('Document directory "%s" could not be created.', $documentDir));
}| @is_dir($dir) || @mkdir($dir, $this->dirMode, true); | ||
| file_put_contents( | ||
| $this->path, | ||
| json_encode($event, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT) . "\n", | ||
| FILE_APPEND | ||
| ); |
There was a problem hiding this comment.
Silently suppressing errors from mkdir and file_put_contents can be problematic for a logger. If logging fails due to permission issues, you won't know about it, which defeats the purpose of logging. It's better to let the errors surface by removing the @ operator and checking function return values. Using trigger_error is a good way to report this without halting execution by throwing an exception.
if (!is_dir($dir) && !mkdir($dir, $this->dirMode, true) && !is_dir($dir)) {
trigger_error("Could not create log directory: {$dir}", E_USER_WARNING);
return;
}
$result = file_put_contents(
$this->path,
json_encode($event, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT) . "\n",
FILE_APPEND
);
if ($result === false) {
trigger_error("Could not write to log file: {$this->path}", E_USER_WARNING);
}|
|
||
| public function replaceDocumentNote(int $leadId, string $template, string $url): void | ||
| { | ||
| @is_dir($this->metaDir) || @mkdir($this->metaDir, 0775, true); |
There was a problem hiding this comment.
The use of the error control operator (@) for mkdir can hide filesystem permission issues. If the directory for metadata cannot be created, file_put_contents will fail, and the root cause might not be obvious. This could lead to issues with note tracking that are hard to debug.
if (!is_dir($this->metaDir) && !mkdir($this->metaDir, 0775, true) && !is_dir($this->metaDir)) {
throw new \RuntimeException(sprintf('Directory "%s" was not created', $this->metaDir));
}| @is_dir($dir) || @mkdir($dir, 0775, true); | ||
| file_put_contents($this->path, json_encode($tokens, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT)); |
There was a problem hiding this comment.
The use of the error control operator (@) for mkdir and the lack of checking the return value of file_put_contents can mask underlying filesystem issues, such as incorrect permissions. This could lead to silent failures where tokens are not saved, causing authorization problems that are difficult to debug. It would be more robust to check the return values and throw an exception on failure.
if (!is_dir($dir) && !mkdir($dir, 0775, true) && !is_dir($dir)) {
throw new \RuntimeException(sprintf('Directory "%s" was not created', $dir));
}
if (file_put_contents($this->path, json_encode($tokens, JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT)) === false) {
throw new \RuntimeException(sprintf('Unable to write to file "%s"', $this->path));
}| @is_dir($this->cacheDir) || @mkdir($this->cacheDir, $this->dirMode, true); | ||
| file_put_contents($this->path($leadId), json_encode([ | ||
| 'saved_at' => time(), | ||
| 'template' => $template, | ||
| 'discount' => $discount, | ||
| 'products' => $products, | ||
| ], JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT)); |
There was a problem hiding this comment.
Using the error control operator (@) here can suppress important filesystem errors. If the cache directory cannot be created, file_put_contents will fail silently (or with a warning that isn't handled), which could lead to data not being cached as expected. Explicit error handling would make this more robust.
if (!is_dir($this->cacheDir) && !mkdir($this->cacheDir, $this->dirMode, true) && !is_dir($this->cacheDir)) {
throw new \RuntimeException(sprintf('Cache directory "%s" could not be created.', $this->cacheDir));
}
$data = json_encode([
'saved_at' => time(),
'template' => $template,
'discount' => $discount,
'products' => $products,
], JSON_UNESCAPED_UNICODE | JSON_PRETTY_PRINT);
if (file_put_contents($this->path($leadId), $data) === false) {
throw new \RuntimeException(sprintf('Unable to write to cache file: %s', $this->path($leadId)));
}There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7996817dab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private function authMode(): string | ||
| { | ||
| $security = is_array($this->config['security'] ?? null) ? $this->config['security'] : []; | ||
| $mode = (string)($security['generate_auth_mode'] ?? 'browser_token'); |
There was a problem hiding this comment.
Preserve legacy HMAC mode when a secret exists
Existing deployments that followed the previous configuration only have a top-level hmac_secret; before this change, generate.php enforced and accepted X-Signature whenever that value was non-empty. With this default, those same signed requests now fall into browser_token mode and are rejected unless the client also sends a freshly minted generate_token, so upgrading breaks server-to-server generation for any install that has not added the new security.generate_auth_mode key. Default to HMAC/either when the legacy secret is present, or migrate the old key explicitly.
Useful? React with 👍 / 👎.
Summary
User Impact
generate_tokenfrom prefill, while HMAC remains available for trusted server-to-server clients.Test Plan
./vendor/bin/phpunit/.\vendor\bin\phpunit— OK (35 tests, 86 assertions)Notes
config/config.phpremains ignored because it may contain secrets; tracked defaults live inconfig/config.example.php.docs/assets/github-social-preview.png.