Feat/add payroll and local taxes support#1
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds employer/employee sub-resources, payroll (nómina) and local tax examples, stamp transfer/withdraw operations, and related service interfaces; simplifies invoice creation logic and exposes stamp service from the FiscalApi client. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
src/Services/EmployerService.php (1)
43-67: Add fail-fast validation for empty payloads in create/update.At Line 43 and Line 58,
array $datais forwarded without local validation. Rejecting empty payloads early improves error clarity and avoids unnecessary HTTP calls.💡 Proposed refactor
class EmployerService implements EmployerServiceInterface { @@ public function create(string $personId, array $data): FiscalApiHttpResponseInterface { $this->validatePersonId($personId); + $this->validatePayload($data); return $this->httpClient->post( @@ public function update(string $personId, array $data): FiscalApiHttpResponseInterface { $this->validatePersonId($personId); + $this->validatePayload($data); return $this->httpClient->put( @@ protected function validatePersonId(string $personId): void { if (empty(trim($personId))) { throw new InvalidArgumentException('El ID de la persona es obligatorio'); } } + + protected function validatePayload(array $data): void + { + if ($data === []) { + throw new InvalidArgumentException('Los datos del empleador son obligatorios'); + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/EmployerService.php` around lines 43 - 67, Add fail-fast validation in the create and update methods to reject empty payloads: inside EmployerService::create and EmployerService::update, after validatePersonId(...) verify that the incoming array $data is not empty (e.g. empty($data) or count($data) === 0) and throw a meaningful InvalidArgumentException (or a domain-specific exception used in the project) with a clear message like "Payload cannot be empty" so the method returns an immediate client error instead of making the HTTP call.examples/EjemplosImpuestosLocalesReferencias.php (1)
195-210: Consider centralizing repeated example helpers.
consoleLog,consoleError, andgetCurrentDateare repeated across example scripts; moving them to a shared examples bootstrap/helper file would reduce drift.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/EjemplosImpuestosLocalesReferencias.php` around lines 195 - 210, Move the duplicated helper functions consoleLog, consoleError, and getCurrentDate into a single shared examples helper (e.g., ExamplesHelper) and update each example script to include/require that helper instead of redefining them; ensure the helper exports the three functions with the same names, that getCurrentDate still sets the timezone ('Etc/GMT+6') and returns the ISO datetime string, and that consoleLog accepts FiscalApiHttpResponseInterface and consoleError accepts \Exception so existing calls in the examples continue to work without changes.src/Services/StampService.php (1)
28-53:transferStamps()andwithdrawStamps()duplicate the same flow.Both methods execute identical validate+post logic. Consider extracting one private helper to keep a single code path.
♻️ Proposed refactor
public function transferStamps(array $data): FiscalApiHttpResponseInterface { - $this->validateStampTransaction($data); - - return $this->httpClient->post( - $this->buildResourceUrl(), - [ - 'data' => $data - ] - ); + return $this->executeStampTransaction($data); } public function withdrawStamps(array $data): FiscalApiHttpResponseInterface { + return $this->executeStampTransaction($data); + } + + private function executeStampTransaction(array $data): FiscalApiHttpResponseInterface + { $this->validateStampTransaction($data); return $this->httpClient->post( $this->buildResourceUrl(), [ 'data' => $data ] ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/StampService.php` around lines 28 - 53, transferStamps() and withdrawStamps() both validate the payload and POST identical data; extract the shared logic into a private helper (e.g., private function sendStampTransaction(array $data): FiscalApiHttpResponseInterface) that calls validateStampTransaction($data) then returns $this->httpClient->post($this->buildResourceUrl(), ['data' => $data]); replace the bodies of transferStamps() and withdrawStamps() to delegate to this new helper so there is a single code path while preserving existing method signatures and return type.src/Services/InvoiceService.php (1)
30-38:create()is now redundant withAbstractService::create().This override no longer adds behavior and can be removed to avoid drift.
♻️ Proposed simplification
- /** - * {`@inheritdoc`} - */ - public function create(array $data): FiscalApiHttpResponseInterface - { - return $this->httpClient->post( - $this->buildResourceUrl(), - [ - 'data' => $data - ] - ); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/InvoiceService.php` around lines 30 - 38, The InvoiceService::create(array $data): FiscalApiHttpResponseInterface method duplicates behavior already provided by AbstractService::create(), so remove the redundant override from the InvoiceService class and rely on the inherited AbstractService::create() implementation; ensure no unique behavior or callers depend on InvoiceService::create() signature change and run tests to verify nothing breaks.src/Services/PersonService.php (1)
14-15: Nullable types are unnecessary since properties are always initialized in the constructor.The properties are declared as nullable (
?EmployeeServiceInterface,?EmployerServiceInterface) and initialized tonull, but the constructor immediately assigns concrete instances. This creates a type inconsistency: the properties are nullable but the getters return non-nullable types.♻️ Proposed fix to remove unnecessary nullability
- private ?EmployeeServiceInterface $employeeService = null; - private ?EmployerServiceInterface $employerService = null; + private EmployeeServiceInterface $employeeService; + private EmployerServiceInterface $employerService;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/PersonService.php` around lines 14 - 15, The private properties $employeeService and $employerService are declared nullable (?EmployeeServiceInterface and ?EmployerServiceInterface) but are always set in the constructor and their getters return non-nullable types; remove the unnecessary nullability by changing the property types to EmployeeServiceInterface and EmployerServiceInterface (drop the ?), remove the = null initializers, and ensure the constructor assigns the concrete instances to these non-nullable properties so the getters and property types are consistent.src/Services/StampServiceInterface.php (1)
30-36: PHPDoc parameters appear to be copied fromtransferStamps.The
withdrawStampsPHPDoc describes parameters(fromPersonId, toPersonId, amount)which looks like transfer semantics. For a withdrawal operation, verify iftoPersonIdis actually needed, or update the documentation to accurately reflect the expected data structure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/StampServiceInterface.php` around lines 30 - 36, The PHPDoc for withdrawStamps incorrectly lists transfer-style params (fromPersonId, toPersonId, amount); inspect the withdrawStamps implementation and/or the related transferStamps signature and update the docblock for withdrawStamps to reflect the actual expected keys (e.g., personId or fromPersonId and amount) or remove toPersonId if not used; ensure the `@param` array $data description and any inline examples match the actual keys consumed by the withdrawStamps method.src/Services/EmployeeService.php (1)
14-102: Consider extracting common logic to a base class to reduce duplication withEmployerService.
EmployeeServiceandEmployerServiceshare nearly identical implementations—only$subResourcePathdiffers. A common abstract base class could eliminate this duplication.♻️ Proposed abstract base class
abstract class AbstractSubResourceService { protected FiscalApiHttpClientInterface $httpClient; protected string $resourcePath = 'people'; abstract protected function getSubResourcePath(): string; public function __construct(FiscalApiHttpClientInterface $httpClient) { $this->httpClient = $httpClient; } public function get(string $personId): FiscalApiHttpResponseInterface { $this->validatePersonId($personId); return $this->httpClient->get($this->buildSubResourceUrl($personId)); } // ... other common methods protected function buildSubResourceUrl(string $personId): string { return '/' . $this->resourcePath . '/' . urlencode($personId) . '/' . $this->getSubResourcePath(); } protected function validatePersonId(string $personId): void { if (empty(trim($personId))) { throw new InvalidArgumentException('El ID de la persona es obligatorio'); } } }Then simplify
EmployeeService:class EmployeeService extends AbstractSubResourceService implements EmployeeServiceInterface { protected function getSubResourcePath(): string { return 'employee'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Services/EmployeeService.php` around lines 14 - 102, EmployeeService duplicates logic present in EmployerService; extract the shared behavior into an abstract base (e.g., AbstractSubResourceService) that holds $httpClient, $resourcePath, and implements get/post/put/delete, buildSubResourceUrl and validatePersonId, then have EmployeeService and EmployerService extend it and implement a small getSubResourcePath() (or protected string $subResourcePath) to return 'employee' or 'employer' respectively; update constructors to call parent::__construct(FiscalApiHttpClientInterface $httpClient) and remove duplicated methods (get, create, update, delete, buildSubResourceUrl, validatePersonId) from EmployeeService so it only supplies the sub-resource identifier.examples/EjemplosNominaValores.php (2)
1565-1568:getCurrentDate()implementation differs from other example files.This implementation uses
DateTimewithout explicit timezone, while other example files (e.g.,EjemplosImpuestosLocalesValores.php,EjemplosNominaReferencias.php) usedate_default_timezone_set('Etc/GMT+6')before callingdate(). This could produce different timestamps depending on the server's default timezone.♻️ Proposed fix for consistency with other examples
function getCurrentDate(): string { - return (new \DateTime())->format('Y-m-d\TH:i:s'); + date_default_timezone_set('Etc/GMT+6'); + return date('Y-m-d\TH:i:s'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/EjemplosNominaValores.php` around lines 1565 - 1568, The getCurrentDate() function currently creates a DateTime without an explicit timezone which can yield different timestamps across servers; update getCurrentDate() to use the same timezone as other examples by either setting date_default_timezone_set('Etc/GMT+6') before producing the string or by constructing the DateTime with new \DateTimeZone('Etc/GMT+6') and then formatting — ensure the returned format remains 'Y-m-d\TH:i:s' and update the function body in getCurrentDate() accordingly.
1552-1560: Consider extracting shared helper functions to reduce duplication.
consoleLog,consoleError, andgetCurrentDateare duplicated across multiple example files (EjemplosTimbres.php,EjemplosNominaReferencias.php,EjemplosImpuestosLocalesValores.php). Extracting these to a shared helper file would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/EjemplosNominaValores.php` around lines 1552 - 1560, Duplicate utility functions consoleLog, consoleError, and getCurrentDate should be extracted into a single shared helper file (e.g., create a helpers file and define these functions once), then replace the duplicated definitions in EjemplosNominaValores.php, EjemplosTimbres.php, EjemplosNominaReferencias.php, and EjemplosImpuestosLocalesValores.php with a single require_once/include_once of that helper; ensure the helper exports the same function names (consoleLog, consoleError, getCurrentDate) and update the example files to include the helper at top to avoid redeclaration and maintain identical behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/EjemplosImpuestosLocalesValores.php`:
- Around line 24-25: The sample default currently sets SSL certificate
verification to false (the boolean value on Line 25); change this default to
true to enable certificate verification in examples (e.g., update the relevant
config/array entry used by EjemplosImpuestosLocalesValores.php), and add a short
comment or note that disabling verification is only for explicit local testing
with an environment-specific override or flag (do not remove the ability to
disable it for local tests, but keep secure=true as the example default).
- Around line 29-31: The file contains hard-coded secrets ($base64Cert,
$base64Key, $password); remove the embedded certificate, private key and
password and replace them with placeholders that load from a secure source
(e.g., read base64 or file path from environment variables or a protected file).
Update references to $base64Cert, $base64Key and $password to obtain values via
getenv('...') or file_get_contents(getenv('...')) and fail with a clear error if
not provided, ensuring no secret literals remain in the example.
In `@examples/EjemplosNominaReferencias.php`:
- Around line 26-27: The sample TLS/SSL configuration currently contains the
literal values "false, false" (the TLS verification flags in the config array
inside EjemplosNominaReferencias.php); change the verification flag(s) from
false to true so TLS verification is enabled by default (e.g., set the named
option like verify/verify_peer to true if present), add a short inline comment
noting it may only be disabled in controlled local environments, and ensure any
code that conditionally disables verification is gated behind an explicit
local/dev-only flag.
In `@examples/EjemplosTimbres.php`:
- Around line 26-27: Change the example to enable TLS/SSL verification by
default by replacing the boolean for the "Desactivar verificación SSL" option
from false to true (i.e., turn on SSL verification in the example), and update
the accompanying comment to indicate that disabling verification is only for
controlled local debugging; look for the option whose comment reads "Desactivar
verificación SSL" in EjemplosTimbres.php and flip its value to enable
verification.
In `@README.md`:
- Line 428: Replace the misspelled word "factuación" with the correct spelling
"facturación" in the README line that currently reads "Gestión de datos de
empleador (patrón) y empleado asociados a personas, necesarios para factuación
de nómina." so the sentence reads "... necesarios para facturación de nómina.";
locate the string in README.md and update it accordingly.
In `@src/Services/PersonServiceInterface.php`:
- Around line 14-26: The new methods getEmployeeService() and
getEmployerService() on PersonServiceInterface are a breaking change for
external implementers; instead revert those methods from the public
PersonServiceInterface and introduce a new interface (e.g.,
ExtendedPersonServiceInterface) that extends PersonServiceInterface and declares
getEmployeeService() and getEmployerService(); then have the in-repo concrete
PersonService implement ExtendedPersonServiceInterface (or a new class implement
it) so internal code can use the new methods while third-party implementers
remain compatible with the original PersonServiceInterface.
---
Nitpick comments:
In `@examples/EjemplosImpuestosLocalesReferencias.php`:
- Around line 195-210: Move the duplicated helper functions consoleLog,
consoleError, and getCurrentDate into a single shared examples helper (e.g.,
ExamplesHelper) and update each example script to include/require that helper
instead of redefining them; ensure the helper exports the three functions with
the same names, that getCurrentDate still sets the timezone ('Etc/GMT+6') and
returns the ISO datetime string, and that consoleLog accepts
FiscalApiHttpResponseInterface and consoleError accepts \Exception so existing
calls in the examples continue to work without changes.
In `@examples/EjemplosNominaValores.php`:
- Around line 1565-1568: The getCurrentDate() function currently creates a
DateTime without an explicit timezone which can yield different timestamps
across servers; update getCurrentDate() to use the same timezone as other
examples by either setting date_default_timezone_set('Etc/GMT+6') before
producing the string or by constructing the DateTime with new
\DateTimeZone('Etc/GMT+6') and then formatting — ensure the returned format
remains 'Y-m-d\TH:i:s' and update the function body in getCurrentDate()
accordingly.
- Around line 1552-1560: Duplicate utility functions consoleLog, consoleError,
and getCurrentDate should be extracted into a single shared helper file (e.g.,
create a helpers file and define these functions once), then replace the
duplicated definitions in EjemplosNominaValores.php, EjemplosTimbres.php,
EjemplosNominaReferencias.php, and EjemplosImpuestosLocalesValores.php with a
single require_once/include_once of that helper; ensure the helper exports the
same function names (consoleLog, consoleError, getCurrentDate) and update the
example files to include the helper at top to avoid redeclaration and maintain
identical behavior.
In `@src/Services/EmployeeService.php`:
- Around line 14-102: EmployeeService duplicates logic present in
EmployerService; extract the shared behavior into an abstract base (e.g.,
AbstractSubResourceService) that holds $httpClient, $resourcePath, and
implements get/post/put/delete, buildSubResourceUrl and validatePersonId, then
have EmployeeService and EmployerService extend it and implement a small
getSubResourcePath() (or protected string $subResourcePath) to return 'employee'
or 'employer' respectively; update constructors to call
parent::__construct(FiscalApiHttpClientInterface $httpClient) and remove
duplicated methods (get, create, update, delete, buildSubResourceUrl,
validatePersonId) from EmployeeService so it only supplies the sub-resource
identifier.
In `@src/Services/EmployerService.php`:
- Around line 43-67: Add fail-fast validation in the create and update methods
to reject empty payloads: inside EmployerService::create and
EmployerService::update, after validatePersonId(...) verify that the incoming
array $data is not empty (e.g. empty($data) or count($data) === 0) and throw a
meaningful InvalidArgumentException (or a domain-specific exception used in the
project) with a clear message like "Payload cannot be empty" so the method
returns an immediate client error instead of making the HTTP call.
In `@src/Services/InvoiceService.php`:
- Around line 30-38: The InvoiceService::create(array $data):
FiscalApiHttpResponseInterface method duplicates behavior already provided by
AbstractService::create(), so remove the redundant override from the
InvoiceService class and rely on the inherited AbstractService::create()
implementation; ensure no unique behavior or callers depend on
InvoiceService::create() signature change and run tests to verify nothing
breaks.
In `@src/Services/PersonService.php`:
- Around line 14-15: The private properties $employeeService and
$employerService are declared nullable (?EmployeeServiceInterface and
?EmployerServiceInterface) but are always set in the constructor and their
getters return non-nullable types; remove the unnecessary nullability by
changing the property types to EmployeeServiceInterface and
EmployerServiceInterface (drop the ?), remove the = null initializers, and
ensure the constructor assigns the concrete instances to these non-nullable
properties so the getters and property types are consistent.
In `@src/Services/StampService.php`:
- Around line 28-53: transferStamps() and withdrawStamps() both validate the
payload and POST identical data; extract the shared logic into a private helper
(e.g., private function sendStampTransaction(array $data):
FiscalApiHttpResponseInterface) that calls validateStampTransaction($data) then
returns $this->httpClient->post($this->buildResourceUrl(), ['data' => $data]);
replace the bodies of transferStamps() and withdrawStamps() to delegate to this
new helper so there is a single code path while preserving existing method
signatures and return type.
In `@src/Services/StampServiceInterface.php`:
- Around line 30-36: The PHPDoc for withdrawStamps incorrectly lists
transfer-style params (fromPersonId, toPersonId, amount); inspect the
withdrawStamps implementation and/or the related transferStamps signature and
update the docblock for withdrawStamps to reflect the actual expected keys
(e.g., personId or fromPersonId and amount) or remove toPersonId if not used;
ensure the `@param` array $data description and any inline examples match the
actual keys consumed by the withdrawStamps method.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
README.mdexamples/EjemplosImpuestosLocalesReferencias.phpexamples/EjemplosImpuestosLocalesValores.phpexamples/EjemplosNominaReferencias.phpexamples/EjemplosNominaValores.phpexamples/EjemplosTimbres.phpexamples/examples.phpsrc/Services/EmployeeService.phpsrc/Services/EmployeeServiceInterface.phpsrc/Services/EmployerService.phpsrc/Services/EmployerServiceInterface.phpsrc/Services/FiscalApiClient.phpsrc/Services/FiscalApiClientInterface.phpsrc/Services/InvoiceService.phpsrc/Services/InvoiceServiceInterface.phpsrc/Services/PersonService.phpsrc/Services/PersonServiceInterface.phpsrc/Services/StampService.phpsrc/Services/StampServiceInterface.phpsrc/Services/TaxFileServiceInterface.php
Comment on lines
+29
to
+31
| $base64Cert = "MIIFgDCCA2igAwIBAgIUMzAwMDEwMDAwMDA1MDAwMDM0NDYwDQYJKoZIhvcNAQELBQAwggErMQ8wDQYDVQQDDAZBQyBVQVQxLjAsBgNVBAoMJVNFUlZJQ0lPIERFIEFETUlOSVNUUkFDSU9OIFRSSUJVVEFSSUExGjAYBgNVBAsMEVNBVC1JRVMgQXV0aG9yaXR5MSgwJgYJKoZIhvcNAQkBFhlvc2Nhci5tYXJ0aW5lekBzYXQuZ29iLm14MR0wGwYDVQQJDBQzcmEgY2VycmFkYSBkZSBjYWxpejEOMAwGA1UEEQwFMDYzNzAxCzAJBgNVBAYTAk1YMRkwFwYDVQQIDBBDSVVEQUQgREUgTUVYSUNPMREwDwYDVQQHDAhDT1lPQUNBTjERMA8GA1UELRMIMi41LjQuNDUxJTAjBgkqhkiG9w0BCQITFnJlc3BvbnNhYmxlOiBBQ0RNQS1TQVQwHhcNMjMwNTE4MTQzNTM3WhcNMjcwNTE4MTQzNTM3WjCBpzEdMBsGA1UEAxMUS0FSTEEgRlVFTlRFIE5PTEFTQ08xHTAbBgNVBCkTFEtBUkxBIEZVRU5URSBOT0xBU0NPMR0wGwYDVQQKExRLQVJMQSBGVUVOVEUgTk9MQVNDTzEWMBQGA1UELRMNRlVOSzY3MTIyOFBINjEbMBkGA1UEBRMSRlVOSzY3MTIyOE1DTE5MUjA1MRMwEQYDVQQLEwpTdWN1cnNhbCAxMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEAhNXbTSqGX6+/3Urpemyy5vVG2IdP2v7v001+c4BoMxEDFDQ32cOFdDiRxy0Fq9aR+Ojrofq8VeftvN586iyA1A6a0QnA68i7JnQKI4uJy+u0qiixuHu6u3b3BhSpoaVHcUtqFWLLlzr0yBxfVLOqVna/1/tHbQJg9hx57mp97P0JmXO1WeIqi+Zqob/mVZh2lsPGdJ8iqgjYFaFn9QVOQ1Pq74o1PTqwfzqgJSfV0zOOlESDPWggaDAYE4VNyTBisOUjlNd0x7ppcTxSi3yenrJHqkq/pqJsRLKf6VJ/s9p6bsd2bj07hSDpjlDC2lB25eEfkEkeMkXoE7ErXQ5QCwIDAQABox0wGzAMBgNVHRMBAf8EAjAAMAsGA1UdDwQEAwIGwDANBgkqhkiG9w0BAQsFAAOCAgEAHwYpgbClHULXYhK4GNTgonvXh81oqfXwCSWAyDPiTYFDWVfWM9C4ApxMLyc0XvJte75Rla+bPC08oYN3OlhbbvP3twBL/w9SsfxvkbpFn2ZfGSTXZhyiq4vjmQHW1pnFvGelwgU4v3eeRE/MjoCnE7M/Q5thpuog6WGf7CbKERnWZn8QsUaJsZSEkg6Bv2jm69ye57ab5rrOUaeMlstTfdlaHAEkUgLX/NXq7RbGwv82hkHY5b2vYcXeh34tUMBL6os3OdRlooN9ZQGkVIISvxVZpSHkYC20DFNh1Bb0ovjfujlTcka81GnbUhFGZtRuoVQ1RVpMO8xtx3YKBLp4do3hPmnRCV5hCm43OIjYx9Ov2dqICV3AaNXSLV1dW39Bak/RBiIDGHzOIW2+VMPjvvypBjmPv/tmbqNHWPSAWOxTyMx6E1gFCZvi+5F+BgkdC3Lm7U0BU0NfvsXajZd8sXnIllvEMrikCLoI/yurvexNDcF1RW/FhMsoua0eerwczcNm66pGjHm05p9DR6lFeJZrtqeqZuojdxBWy4vH6ghyJaupergoX+nmdG3JYeRttCFF/ITI68TeCES5V3Y0C3psYAg1XxcGRLGd4chPo/4xwiLkijWtgt0/to5ljGBwfK7r62PHZfL1Dp+i7V3w7hmOlhbXzP+zhMZn1GCk7KY="; | ||
| $base64Key = "MIIFDjBABgkqhkiG9w0BBQ0wMzAbBgkqhkiG9w0BBQwwDgQIAgEAAoIBAQACAggAMBQGCCqGSIb3DQMHBAgwggS9AgEAMASCBMh4EHl7aNSCaMDA1VlRoXCZ5UUmqErAbucRBAKNQXH8t8gVCl/ItHMI2hMJ76QOECOqEi1Y89cDpegDvh/INXyMsXbzi87tfFzgq1O+9ID6aPWGg+bNGADXyXxDVdy7Nq/SCdoXvo66MTYwq8jyJeUHDHEGMVBcmZpD44VJCvLBxDcvByuevP4Wo2NKqJCwK+ecAdZc/8Rvd947SjbMHuS8BppfQWARVUqA5BLOkTAHNv6tEk/hncC7O2YOGSShart8fM8dokgGSyewHVFe08POuQ+WDHeVpvApH/SP29rwktSoiHRoL6dK+F2YeEB5SuFW9LQgYCutjapmUP/9TC3Byro9Li6UrvQHxNmgMFGQJSYjFdqlGjLibfuguLp7pueutbROoZaSxU8HqlfYxLkpJUxUwNI1ja/1t3wcivtWknVXBd13R06iVfU1HGe8Kb4u5il4a4yP4p7VT4RE3b1SBLJeG+BxHiE8gFaaKcX/Cl6JV14RPTvk/6VnAtEQ66qHJex21KKuiJo2JoOmDXVHmvGQlWXNjYgoPx28Xd5WsofL+n7HDR2Ku8XgwJw6IXBJGuoday9qWN9v/k7DGlNGB6Sm4gdVUmycMP6EGhB1vFTiDfOGQO42ywmcpKoMETPVQ5InYKE0xAOckgcminDgxWjtUHjBDPEKifEjYudPwKmR6Cf4ZdGvUWwY/zq9pPAC9bu423KeBCnSL8AQ4r5SVsW6XG0njamwfNjpegwh/YG7sS7sDtZ8gi7r6tZYjsOqZlCYU0j7QTBpuQn81Yof2nQRCFxhRJCeydmIA8+z0nXrcElk7NDPk4kYQS0VitJ2qeQYNENzGBglROkCl2y6GlxAG80IBtReCUp/xOSdlwDR0eim+SNkdStvmQM5IcWBuDKwGZc1A4v/UoLl7niV9fpl4X6bUX8lZzY4gidJOafoJ30VoY/lYGkrkEuz3GpbbT5v8fF3iXVRlEqhlpe8JSGu7Rd2cPcJSkQ1Cuj/QRhHPhFMF2KhTEf95c9ZBKI8H7SvBi7eLXfSW2Y0ve6vXBZKyjK9whgCU9iVOsJjqRXpAccaWOKi420CjmS0+uwj/Xr2wLZhPEjBA/G6Od30+eG9mICmbp/5wAGhK/ZxCT17ZETyFmOMo49jl9pxdKocJNuzMrLpSz7/g5Jwp8+y8Ck5YP7AX0R/dVA0t37DO7nAbQT5XVSYpMVh/yvpYJ9WR+tb8Yg1h2lERLR2fbuhQRcwmisZR2W3Sr2b7hX9MCMkMQw8y2fDJrzLrqKqkHcjvnI/TdzZW2MzeQDoBBb3fmgvjYg07l4kThS73wGX992w2Y+a1A2iirSmrYEm9dSh16JmXa8boGQAONQzQkHh7vpw0IBs9cnvqO1QLB1GtbBztUBXonA4TxMKLYZkVrrd2RhrYWMsDp7MpC4M0p/DA3E/qscYwq1OpwriewNdx6XXqMZbdUNqMP2viBY2VSGmNdHtVfbN/rnaeJetFGX7XgTVYD7wDq8TW9yseCK944jcT+y/o0YiT9j3OLQ2Ts0LDTQskpJSxRmXEQGy3NBDOYFTvRkcGJEQJItuol8NivJN1H9LoLIUAlAHBZxfHpUYx66YnP4PdTdMIWH+nxyekKPFfAT7olQ="; | ||
| $password = "12345678a"; |
There was a problem hiding this comment.
Do not embed certificate/private-key material in repository examples.
At Line 29–31, the example includes full key material and password inline. Even for demos, this encourages secret sprawl and accidental reuse. Replace with placeholders and load from secure env/files.
🔐 Suggested change
-$base64Cert = "MIIFgDCC...";
-$base64Key = "MIIFDjBA...";
-$password = "12345678a";
+$base64Cert = getenv('FISCALAPI_TEST_CERT_BASE64') ?: '<base64-cert>';
+$base64Key = getenv('FISCALAPI_TEST_KEY_BASE64') ?: '<base64-key>';
+$password = getenv('FISCALAPI_TEST_KEY_PASSWORD') ?: '<key-password>';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/EjemplosImpuestosLocalesValores.php` around lines 29 - 31, The file
contains hard-coded secrets ($base64Cert, $base64Key, $password); remove the
embedded certificate, private key and password and replace them with
placeholders that load from a secure source (e.g., read base64 or file path from
environment variables or a protected file). Update references to $base64Cert,
$base64Key and $password to obtain values via getenv('...') or
file_get_contents(getenv('...')) and fail with a clear error if not provided,
ensuring no secret literals remain in the example.
Comment on lines
+26
to
+27
| false, | ||
| false, |
There was a problem hiding this comment.
Keep TLS verification enabled in sample configuration.
At Line 27, SSL verification is disabled. Use true by default and only disable in controlled local environments.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/EjemplosNominaReferencias.php` around lines 26 - 27, The sample
TLS/SSL configuration currently contains the literal values "false, false" (the
TLS verification flags in the config array inside
EjemplosNominaReferencias.php); change the verification flag(s) from false to
true so TLS verification is enabled by default (e.g., set the named option like
verify/verify_peer to true if present), add a short inline comment noting it may
only be disabled in controlled local environments, and ensure any code that
conditionally disables verification is gated behind an explicit local/dev-only
flag.
There was a problem hiding this comment.
@Anidem1995, understood. I acknowledge your assessment on this example configuration.
Comment on lines
+26
to
+27
| false, // Imprimir raw request / response | ||
| false, // Desactivar verificación SSL |
There was a problem hiding this comment.
Use TLS verification by default in examples.
At Line 27, SSL verification is disabled. This weakens transport security if copied as-is; keep it enabled by default and document disabling only for controlled local debugging.
🔒 Suggested change
- false, // Desactivar verificación SSL
+ true, // Mantener verificación SSL (desactivar solo en pruebas locales controladas)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| false, // Imprimir raw request / response | |
| false, // Desactivar verificación SSL | |
| false, // Imprimir raw request / response | |
| true, // Mantener verificación SSL (desactivar solo en pruebas locales controladas) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@examples/EjemplosTimbres.php` around lines 26 - 27, Change the example to
enable TLS/SSL verification by default by replacing the boolean for the
"Desactivar verificación SSL" option from false to true (i.e., turn on SSL
verification in the example), and update the accompanying comment to indicate
that disabling verification is only for controlled local debugging; look for the
option whose comment reads "Desactivar verificación SSL" in EjemplosTimbres.php
and flip its value to enable verification.
There was a problem hiding this comment.
@Anidem1995, you're right. Since this is an example file for testing purposes ("Ambiente de pruebas"), having SSL verification disabled is appropriate for developers working with test environments. I apologize for the noise.
Comment on lines
+14
to
+26
| /** | ||
| * Obtiene el sub-servicio de datos de empleado. | ||
| * | ||
| * @return EmployeeServiceInterface | ||
| */ | ||
| public function getEmployeeService(): EmployeeServiceInterface; | ||
|
|
||
| /** | ||
| * Obtiene el sub-servicio de datos de empleador (patrón). | ||
| * | ||
| * @return EmployerServiceInterface | ||
| */ | ||
| public function getEmployerService(): EmployerServiceInterface; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find all in-repo implementations of PersonServiceInterface
rg -nP --type=php 'implements\s+PersonServiceInterface'Repository: FiscalAPI/fiscalapi-php
Length of output: 178
🏁 Script executed:
#!/bin/bash
# Check if PersonService.php implements the new methods
sed -n '31,42p' src/Services/PersonService.php
# Also search for any other implementations (mocks, test doubles)
rg -nP --type=php 'class.*implements.*PersonServiceInterface' -A 5Repository: FiscalAPI/fiscalapi-php
Length of output: 761
Adding methods to a public interface is a breaking change for third-party implementations.
The new methods getEmployeeService() and getEmployerService() require all existing third-party implementations of PersonServiceInterface to implement them immediately. While in-repo implementation (PersonService) has been updated correctly, this remains a breaking change if the SDK is used by external projects. If this repository follows SemVer, this should be released as a major version bump or introduced via a new extended interface to maintain backward compatibility.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Services/PersonServiceInterface.php` around lines 14 - 26, The new
methods getEmployeeService() and getEmployerService() on PersonServiceInterface
are a breaking change for external implementers; instead revert those methods
from the public PersonServiceInterface and introduce a new interface (e.g.,
ExtendedPersonServiceInterface) that extends PersonServiceInterface and declares
getEmployeeService() and getEmployerService(); then have the in-repo concrete
PersonService implement ExtendedPersonServiceInterface (or a new class implement
it) so internal code can use the new methods while third-party implementers
remain compatible with the original PersonServiceInterface.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation