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

Fix file caching in JsonFileInterpreter. #452

Open
wants to merge 1 commit into
base: 1.10
Choose a base branch
from

Conversation

cancan101
Copy link
Contributor

JSON version of PR #438
Pulled out of #437 (comment)

CC @fashxp

@fashxp fashxp changed the base branch from 1.x to 1.10 February 21, 2025 11:14
@fashxp fashxp changed the base branch from 1.10 to 1.x February 21, 2025 11:14
@fashxp fashxp added the Bug label Feb 21, 2025
@fashxp
Copy link
Member

fashxp commented Feb 21, 2025

since this is a bugfix, please rebase to 1.10

@fashxp
Copy link
Member

fashxp commented Feb 21, 2025

just for the record: the file caching makes sense because methods fileValid and previewData or doInterpretFileAndCallProcessRow are called after each other. to aviod double loading of the data source, this cache was introduced (but never worked properly it seems ;-) )

@fashxp fashxp added this to the 1.10.1 milestone Feb 21, 2025
@cancan101 cancan101 changed the base branch from 1.x to 1.10 February 21, 2025 16:01
@blankse
Copy link
Contributor

blankse commented Feb 21, 2025

@cancan101 @fashxp I think we should write the cache also in the loadData method and the fileValid should use the loadData method. Now it has redundant loading code. WDYT?

@cancan101
Copy link
Contributor Author

I think there is more that can be done with the caching and I would also suggest reviewing the control flow for error handing (e.g. pass JSON_THROW_ON_ERROR to json_decode and doing so at both call points).

That said, these changes are becoming too much for me to take on.

@@ -32,7 +32,7 @@ class JsonFileInterpreter extends AbstractInterpreter

protected function loadData(string $path): array
{
if ($this->cachedFilePath === $path && !empty($this->cachedContent)) {
if ($this->cachedFilePath !== $path || empty($this->cachedContent)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Variable exists always. We should compare it with null.

Suggested change
if ($this->cachedFilePath !== $path || empty($this->cachedContent)) {
if ($this->cachedFilePath !== $path || $this->cachedContent === null) {

@cancan101
Copy link
Contributor Author

cancan101 commented Feb 21, 2025

@blankse the suggestions might be easier after this PR: https://github.com/pimcore/data-importer/pull/437/files#diff-a36a911bb91e74bc3b35f4779ac1a1dd970ab78d93b7df9d4b2df64846259d9fR45-R57 with the load factored out and then can change such that both loads use that function.

@blankse
Copy link
Contributor

blankse commented Feb 21, 2025

@cancan101 But there is also the error handling and the cache writing only in the fileValid method. I think this should be also moved in the loading method.

@cancan101
Copy link
Contributor Author

yes see my linked PR and comment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants