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

Support localized calculated fields in object bricks #5390

Conversation

BlackbitDevs
Copy link
Contributor

@BlackbitDevs BlackbitDevs commented Dec 5, 2019

With this PR it is possible to have localized calculated value fields in object bricks.

Resolves #5337

@brusch brusch added this to the 6.4.0 ("Stout") milestone Dec 12, 2019
@weisswurstkanone weisswurstkanone removed this from the 6.4.0 ("Stout") milestone Dec 20, 2019
@weisswurstkanone
Copy link
Contributor

could you resolve the conflicts please ? thx

@@ -338,12 +344,32 @@ public function getGetterCodeLocalizedfields($class)
$code .= "\t\t" . '}' . "\n";
$code .= "\t" . '}' . "\n";

if($class instanceof DataObject\Objectbrick\Definition) {
$ownerType = 'objectbrick';
$ownerName = $class->getClassDefinitions()[0]['fieldname']; // todo this is not really correct. Actually we need to find out the name of the brick container (an instance of \Pimcore\Model\DataObject\Objectbrick). This must happen during runtime as one brick can be used in multiple brick containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the right interpretation of the comment ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This means that we cannot determine the name of the class with the brick container field when the brick gets saved. Actually we need to add some code in the generated getter function to determine the brick container field during runtime but I did not find a way to do that because the generated brick class does not know anything in which brick container / data object class it is being used.
And because of that I chose a way to at least enhance the status quo (although this solution is not perfect) by setting the $ownerName to the first configured brick container field of the object brick.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi, just noticed something.

foreach ($result['data'] as $language => &$data) {

The calculator only gets called if there are other fields with data in the localized field container?

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just take it from $this ?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is fieldname the calculated value field in the brick or the brick container field in the data object class? I need the latter.

Copy link
Contributor

Choose a reason for hiding this comment

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

the one in the data object class

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, then have overseen this. Thanks, I will change it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. My test was positive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also check this "I noticed something" thing ?

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Dec 20, 2019

The calculator only gets called if there are other fields with data in the localized field container?

What do you think?

Could you give this a try? The customer which I needed this for has Pimcore 5.8 - there it worked: I have only calculated value fields in the localizedfields container in the brick.

I currently have not the time to set up a Pimcore 6 with a localized calculated value in a brick. Could you please give it a try, @weisswurstkanone - if it does not work I will revert the last commit.

@weisswurstkanone
Copy link
Contributor

It bombs out here because there is no container, $context is empty

image

@BlackbitDevs
Copy link
Contributor Author

BlackbitDevs commented Dec 20, 2019

Forgot the context for getFieldDefinitions() - now it is the same call as in Localizedfield::getLazyLoadedFieldNames(). Better?

@weisswurstkanone
Copy link
Contributor

looks OK from my side

@weisswurstkanone weisswurstkanone merged commit 20dcbaf into pimcore:master Dec 23, 2019
@weisswurstkanone
Copy link
Contributor

thanks a lot!

@weisswurstkanone weisswurstkanone added this to the 6.4.0 ("Stout") milestone Dec 23, 2019
fashxp pushed a commit that referenced this pull request Dec 27, 2019
weisswurstkanone pushed a commit that referenced this pull request Dec 30, 2019
* support localized calculated fields in object bricks

* support localized calculated fields in object bricks

* bugfix: localized calculated field in object class

* set brick container field as ownerName

* calculate value also if no real data fields are in localized field collection

* calculate value also if no real data fields are in localized field collection
weisswurstkanone pushed a commit that referenced this pull request Dec 30, 2019
weisswurstkanone pushed a commit that referenced this pull request Jan 2, 2020
* support localized calculated fields in object bricks

* support localized calculated fields in object bricks

* bugfix: localized calculated field in object class

* set brick container field as ownerName

* calculate value also if no real data fields are in localized field collection

* calculate value also if no real data fields are in localized field collection
weisswurstkanone pushed a commit that referenced this pull request Jan 2, 2020
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.

[Calculated value] Context for localized calculated field in object brick
3 participants