Skip to content
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

TASK: Modernize Doctrine array types #2900

Merged
merged 4 commits into from Feb 13, 2023

Conversation

kdambekalns
Copy link
Member

Update the code of JsonArrayType and ObjectArray to follow modern PHP practices.

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked wit !!! and have upgrade-instructions

@kdambekalns
Copy link
Member Author

kdambekalns commented Sep 13, 2022

Doctrine\DBAL\Platforms\PostgreSQL94Platform depends on class or interface doctrine\dbal\platforms\postgresqlplatform that does not exist

That psalm error is a) not reproducible locally and b) nonsense. And because of a) I cannot even do a baseline update to "fix" it. 🤷‍♂️

Copy link
Member

@mhsdesign mhsdesign left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good by reading so far

kdambekalns and others added 2 commits September 14, 2022 16:52
Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>
Co-authored-by: Marc Henry Schultz <85400359+mhsdesign@users.noreply.github.com>
@kitsunet kitsunet changed the base branch from 8.2 to 8.3 February 13, 2023 14:03
@kitsunet
Copy link
Member

Merging after tests are done.

@kitsunet kitsunet merged commit bae70a0 into neos:8.3 Feb 13, 2023
@kdambekalns kdambekalns deleted the task/modernize-arraytypes branch February 16, 2023 07:49
* @var PersistenceManagerInterface
*/
protected $persistenceManager;
private const FLOW_JSON_ARRAY = 'flow_json_array';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The use of private results in a regression:

➜  neos-manufacture-highest git:(8.3) ✗ ./flow site:import Neos.Demo   
This command can use a lot of memory when importing sites with many resources.
If the import is successful, you will see a message saying "Import of site ... finished".
If you do not see this message, the import failed, most likely due to insufficient memory.
Increase the memory_limit configuration parameter of your php CLI to attempt to fix this.
Starting import...
---
Cannot access private constant Neos\Flow\Persistence\Doctrine\DataTypes\JsonArrayType::FLOW_JSON_ARRAY

  Type: Error
  File: Data/Temporary/Development/Cache/Code/Flow_Object_Classes/Neos_ContentRepos
        itory_Domain_Service_ImportExport_NodeImportService.php
  Line: 642

@mhsdesign
Copy link
Member

Since convertToPHPValue now always returns an array, null will raise an error:

Exception #XXX in line 468 of /Packages/Libraries/doctrine/orm/lib/Doctrine/ORM/Internal/Hydration/AbstractHydrator.php: The JsonArrayType only converts arrays, null given
77 Neos\Flow\Persistence\Doctrine\DataTypes\JsonArrayType::convertToPHPValue(NULL, Doctrine\DBAL\Platforms\PostgreSQL100Platform)
76 Doctrine\ORM\Internal\Hydration\AbstractHydrator::gatherRowData(array|21|, array|1|, array|1|)
75 Doctrine\ORM\Internal\Hydration\ObjectHydrator::hydrateRowData(array|21|, array|0|)
74 Doctrine\ORM\Internal\Hydration\ObjectHydrator::hydrateAllData()

i think it can be tracked down to our nullable json_array_type

/**
 * @ORM\Column(type="flow_json_array",nullable=true)
 * @var array|null
 */
protected ?array $value = [];

fetching the entities from the repository doesnt work anymore with this change. Has anyone else experienced this problem or are we using the api wrong? ^^

@kdambekalns
Copy link
Member Author

Is "our nullable json_array_type" in Neos? Or is "our" code in this case your code?

Anyway, if the implementation of the type did return null before, and it still could do that (e.g. no interface prohibiting that), it could probably be allowed (again), to do so.

@mhsdesign
Copy link
Member

No its in a clients code base ;)

So i created a pr and will test the patch in the project now #3135

neos-bot pushed a commit to neos/flow that referenced this pull request Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants