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 database error when filtering in localized field in the object search #16689

Conversation

saschabaecher-twocream
Copy link
Contributor

Situation:

  • Open an "Event" object in the demo.
  • Go to the "Cars" tab and start the search in the "Cars" relation.
  • Activate the filtering in the "Name" column.
  • An error message appears in the database.

Expected Behavior:

It should be possible to filter in the localized fields in the search as usual.

Fix:

In which we check whether this field is contained in the Localizedfields Field Definition.

Copy link

Review Checklist

  • Target branch (11.1 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone


if (
$definitionExists
&& !$class->getFieldDefinitions()['localizedfields']->getFieldDefinition($paramConditionObject['property'])
Copy link
Contributor

Choose a reason for hiding this comment

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

@saschabaecher-twocream Can you test it with a class that does not have a localized field?

I think there is no localizedfields array key and an exception will occur.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @blankse, thank you for your feedback!

If there are no localized fields in the class, an error occurs. I have now adjusted this.

@kingjia90 kingjia90 deleted the branch pimcore:11.2 March 12, 2024 15:13
@kingjia90 kingjia90 closed this Mar 12, 2024
@kingjia90 kingjia90 reopened this Mar 13, 2024
Copy link

sonarcloud bot commented Mar 13, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@kingjia90 kingjia90 changed the base branch from 11.1 to 11.2 March 13, 2024 07:39
@mattamon
Copy link
Contributor

@saschabaecher-twocream could you please fix PHPStan Static Analysis.

Copy link
Contributor

@mattamon mattamon left a comment

Choose a reason for hiding this comment

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

@saschabaecher-twocream There are still some errors in the static analysis. Could you please fix them. We unfortunately cannot merge this with broken tests.

Thanks in advance!

@mattamon mattamon self-assigned this Apr 19, 2024
Copy link

sonarcloud bot commented Apr 19, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@blankse
Copy link
Contributor

blankse commented Apr 21, 2024

@saschabaecher-twocream I doesn't like the method_exists(). I create a alternative PR:
#16971
I think this is cleaner and more logical.

@mattamon
Copy link
Contributor

@saschabaecher-twocream did you have a chance to check @blankse alternative PR?
Would it be suitable for you too?

I checked it and would also prefer the alternative.

@saschabaecher-twocream
Copy link
Contributor Author

Hi @mattamon, the alternative looks good too! Take the alternative. 👍

@mattamon
Copy link
Contributor

@saschabaecher-twocream thanks for being so casual about it! We still appreciate the work and effort you put into this PR and would be glad to get other PRs in the future!

Have a great weekend!

@mattamon mattamon closed this Apr 26, 2024
@mattamon
Copy link
Contributor

mattamon commented May 3, 2024

@saschabaecher-twocream I merged #16971 and it will be released with 11.2.4.

Have a nice weekend!

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.

None yet

5 participants