NGSTACK-995 upgrade bundle to ibexa 5 #39
Conversation
…nterface and type-hinted the argument
…he exact type of variable cause of polymorphism
… or 'executeStatement' methods
…undle in 'require-dev' section
| public function storeFieldData(VersionInfo $versionInfo, Field $field, array $context): ?bool | ||
| public function __construct(Gateway $gateway) | ||
| { | ||
| $this->gateway = $gateway; |
There was a problem hiding this comment.
In field_types.yaml file, the injected class here is DoctrineStorage, which extends an abstract class Gateway. I added the constructor here because of 'potentially polymorphic call` warning.
There was a problem hiding this comment.
Where did the warning come from? The constructor is already in Ibexa\Contracts\Core\FieldType\GatewayBasedStorage abstract class and it doesn't make sense to repeat it here.
|
|
||
| public function handle(CriteriaConverter $converter, QueryBuilder $queryBuilder, Criterion $criterion, array $languageSettings): string | ||
| { | ||
| /** |
There was a problem hiding this comment.
If you look at the method declaration in the abstract class, it has a PHPDoc which says that it throws an InvalidArgumentException. Also, in the method itself, the $this->getFieldDefinitionIds() method is called, which throws an InvalidArgumentException.
I added the PHPDoc first and foremost to say that the CriterionInterface param is actually EnhancedSelectionCriterion class. If that's alright with you, I would just keep this info in PHPDoc and remove everything else.
There was a problem hiding this comment.
That's fine.
As for @throws, we should not specify those if the method does not directly throw. Otherwise, we might put every throw on every method ever :D
|
|
||
| public function visit(Criterion $criterion, ?CriterionVisitor $subVisitor = null): string | ||
| /** | ||
| * @param EnhancedSelectionCriterion $criterion |
There was a problem hiding this comment.
The @param here is needed because it specifies that the CriterionInterface param is actually an instance of EnhancedSelectionCriterion class
| /** | ||
| * @param EnhancedSelectionCriterion $criterion | ||
| * | ||
| * @throws InvalidArgumentException |
There was a problem hiding this comment.
May I ask why? I understand that there might be cases where it would be better (clearer) to use FQCN in PHPDocs, but here both classes are already imported and used elsewhere in this class.
There was a problem hiding this comment.
Couple of reasons:
- It's our coding standard
- Consistency with cases where classes are not already imported
- It is much clearer and more readable to have a FQCN here, and it keeps code separate from comments.
2 participants
Updated the whole bundle to use Ibexa v5. Fixed a few small things - mostly changed some method definitions and updated database gateway.
Also, I've removed the ibexa-forms-bundle from require-dev section and included ibexa/solr bundle there because it is used in
bundle/Core/Search/Solr/Query/CriterionVisitor/EnhancedSelection.phpfile.In
tests.ymlfile (used for GitHub Actions), the PHP version has been updated to 8.3 and Symfony version to 7.3. Right now, the tests are failing because the ibexa-forms-bundle is removed fromcomposer.jsonfile.