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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HP-1751 Create configurations for billing types #93
base: master
Are you sure you want to change the base?
HP-1751 Create configurations for billing types #93
Changes from 40 commits
01dc6ff
f819552
f5292a4
ccb0b9c
9f7c0da
83d242f
0e26ed5
ff69913
1ac3f7d
99371a4
01e778a
e95efbc
1e731a9
ef63f8f
e72ae0f
94f7fb3
93c0ea8
7972157
e967f1b
944f07d
821f83e
9bc5a8e
b5065b0
0247a8b
e2fd487
48a032e
bc62b19
ea5c85f
02b3aee
a7d1c34
baab974
18c7690
e65d27d
fea22bd
f6c5c1c
9b8bb1d
d9a5ef7
2bb5310
881679f
1acbb7b
0c060ab
93686bb
77b9efe
9dad8c6
285ae11
afe5a62
57394fe
b649b44
2cc1971
6411325
0328f42
1f7fb11
1cfa210
c75b5d4
27c16d1
6808daf
413f729
d7d838a
2c6a195
6f0783b
0e0bc7c
75ee687
9b25ddf
7d09102
c00ea1e
4ce2b38
7269bf9
03ee8dc
97f0d4f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace external references from
hiqdev\billing\registry\...
with internal interfaces if possible.Based on your learnings, the
hiqdev/php-billing
repository should avoid depending onadvancedhosters/billing-registry
. Consider moving or replicating any needed definitions internally.📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove dependencies on external packages.
These imports reference classes from the
hiqdev\billing\registry
namespace, which goes against the project requirement thathiqdev/php-billing
should not depend onadvancedhosters/billing-registry
. This was previously noted in the learnings.Replace these external dependencies with internal interfaces and implementations:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add input validation and fix return type hint
The method needs validation for the representation class and updated return type documentation.
Apply these improvements:
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 42-42: UndefinedDocblockClass
src/product/BillingRegistry.php:42:16: UndefinedDocblockClass: Docblock-defined class, interface or enum named hiqdev\billing\registry\invoice\RepresentationInterface does not exist (see https://psalm.dev/200)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Refactor behavior management for better maintainability
The behavior management implementation needs several improvements:
Apply these improvements:
📝 Committable suggestion
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 104-104: UndefinedClass
src/product/BillingRegistry.php:104:22: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\behavior\TariffTypeBehaviorCollection does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
The
Aggregate
class appears to be missing from the codebaseThe class
Aggregate
is imported fromhiqdev\billing\registry\product\Aggregate
namespace in bothBillingRegistry.php
andPriceTypeDefinition.php
, but:hiqdev/billing-registry
, but this package is not listed in composer dependenciesThis could lead to runtime errors. You should either:
Aggregate
class in the correct namespace if it's meant to be part of this package🔗 Analysis chain
Validate existence of
Aggregate
class prior to usage.Aggregate
is imported fromhiqdev\billing\registry\product\Aggregate
, but static analysis flags it as undefined. Ensure the class is correctly included in composer or located in the repository. Otherwise, reference the correct namespace or rename as needed.Let me search for more information about the Aggregate class.
Let me check if this is a dependency from composer by looking at composer.json.
Let me try one more search with proper escaping to find any references to this class.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 130
Script:
Length of output: 209
Script:
Length of output: 3132
Script:
Length of output: 248
🧰 Tools
🪛 GitHub Check: PHP 8.3
[failure] 121-121: UndefinedClass
src/product/BillingRegistry.php:121:49: UndefinedClass: Class, interface or enum named hiqdev\billing\registry\product\Aggregate does not exist (see https://psalm.dev/019)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Integration verification reveals potential issues
The codebase analysis shows a potential mismatch in the implementation. While
InvoiceDescriptionsBuilder
callsrepresentInvoice()
on price types, the actual implementation shows:BillingRegistry::priceTypes()
exists but returns a GeneratorPriceTypesCollection
andPriceTypeDefinition
PriceTypeDefinition
hasinvoiceRepresentation()
method, but norepresentInvoice()
methodrepresentInvoice()
methodThis suggests that either:
representInvoice()
method is missing from the implementation🔗 Analysis chain
Verify integration with dependent components.
Let's ensure the required methods exist in the dependent classes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 204
Script:
Length of output: 4025
Script:
Length of output: 1992
Script:
Length of output: 1407
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Add error handling and consider using array_map.
The build method could benefit from several improvements:
📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update property type to use internal interface.
This property uses a class from
hiqdev\billing\registry
which goes against the project requirement not to depend on external registry packages.Ensure that
DocumentRepresentationCollection
is defined within this project's namespace.📝 Committable suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update instantiation to use internal implementation.
This creates an instance of an external package class.
Ensure a compatible implementation exists within this project's namespace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Validate or initialize
$description
.Currently,
$description
may remain unset if a caller never invokes thedescription()
method. Consider assigning a default value in the constructor or validating it before access to avoid uninitialized property errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Guard against uninitialized
$unit
increateQuantityFormatter
.If
unit()
is never called before invokingcreateQuantityFormatter()
,$this->unit
will be uninitialized. Add an early check or default assignment to prevent runtime errors.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
🛠️ Refactor suggestion
Remove unused
measuredWith
methodThe
measuredWith
method inPriceTypeDefinition.php
is not used anywhere in the codebase, and its implementation is a no-op that just returns$this
. Since both the method and its parameter typeRcpTrafCollector
are only referenced in this single location, it's safe to remove this unused method.🔗 Analysis chain
Implement measuredWith method or remove it.
The measuredWith method is currently a no-op. Either implement the measurement logic or remove the method if it's not needed.
Let me check if this method is used elsewhere:
Let me try to find any usage of this method or the collector class it depends on.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
Length of output: 439
Script:
Length of output: 327
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Translate documentation to English
The PHPDoc comment is written in a non-English language. For better maintainability and collaboration, documentation should be in English.
Apply this diff:
📝 Committable suggestion