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

[Properties] Set cpath when using AbstractElement::setProperty() #15861

Merged
merged 3 commits into from Sep 25, 2023

Conversation

BlackbitDevs
Copy link
Contributor

When calling $element->setProperty('test', 'text', 'value') the field cpath does not get set. Thus you will get the error
E_DEPRECATED: strcmp(): Passing null to parameter #1 ($string1) of type string is deprecated in /var/www/html/vendor/pimcore/pimcore/models/DataObject/AbstractObject/Dao.php, line 254
in

return strcmp($left['cpath'], $right['cpath']);

This PR on the one hand sets the cpath when using setProperty() and on the other hand prevents above deprecation message if it is not set.

@github-actions
Copy link

github-actions bot commented Aug 30, 2023

Review Checklist

  • Target branch (11.0 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

@dvesh3 dvesh3 added the Bug label Aug 31, 2023
@robertSt7 robertSt7 self-assigned this Sep 21, 2023
@robertSt7 robertSt7 added this to the 11.0.10 milestone Sep 21, 2023
@robertSt7
Copy link
Contributor

@BlackbitDevs Sorry, but I am not able to reproduce it. For me the cpath is always set. Could you please provide all steps to reproduce it? Thanks

@BlackbitDevs
Copy link
Contributor Author

@robertSt7 Sorry, it works when the element gets saved - like for data objects in

$property->setCpath($this->getRealFullPath());

But it does not work when calling $property->save() directly, e.g.

$property = new \Pimcore\Model\Property();
$property = new Property();
$property->setValues([
    'cid' => 123,
    'ctype' => 'object',
    'type' => 'text',
    'name' => 'test',
    'data' => 'value',
    'inheritable' => 0,
    'inherited' => 0
]);
$property->save();

$object = \Pimcore\Model\DataObject::getById(123); // will trigger deprecation error

So my PR description was not correct, you are right. But following defensive programming it should be able to handle null values for cpath as this column is nullable. So no matter HOW such values could get into database, there should not be an error if there is null.
Have adjusted this PR to fall back to retrieving the cpath from the owner element when it is not set.

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dvesh3 dvesh3 modified the milestones: 11.0.10, 11.0.11 Sep 25, 2023
@robertSt7 robertSt7 merged commit d503f0e into pimcore:11.0 Sep 25, 2023
14 checks passed
@robertSt7
Copy link
Contributor

@BlackbitDevs Thanks for the fix :)

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

Successfully merging this pull request may close these issues.

None yet

3 participants