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

Add ObjectField Operator for extracting fields from Data Objects. #436

Open
wants to merge 5 commits into
base: 1.x
Choose a base branch
from

Conversation

cancan101
Copy link
Contributor

@fashxp
Copy link
Member

fashxp commented Nov 21, 2024

very nice, thx! will check in detail as soon we get some time.

two things I noticed right away:

thx again.

@cancan101
Copy link
Contributor Author

cancan101 commented Nov 21, 2024

in JS we should specific translation keys for the fieldLabel - to be independent and more flexible when choosing more descriptive field labels.

What do you mean here? The translation keys I chose exist and are used by: https://pimcore.com/docs/pimcore/10.1/User_Documentation/DataObjects/Grid_Configuration_Operators/Extractors/AnyGetter.html

forward_parameter and attribute have the same meaning here as they do in the AnyGetter

Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
2 New Major Issues (required ≤ 0)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

@cancan101
Copy link
Contributor Author

@fashxp can we get this merged? lmk if i need to clean up the number of return. imho the current form is more readable than collapsing the first two returns.

@fashxp
Copy link
Member

fashxp commented Feb 17, 2025

@cancan101 could you please once more merge 1.x branch an see if tests become green then? Thx.

@cancan101
Copy link
Contributor Author

rebased onto 1.x. I am getting this error: This method has 4 returns, which is more than the 3 allowed. I can "fix" that by combining returns if you think that is more readable.

@fashxp
Copy link
Member

fashxp commented Feb 17, 2025

I accepted sonar cloud, but phpstan needs to be fixed...

public function process(mixed $inputData, bool $dryRun = false): mixed
{
if (!$inputData instanceof ElementInterface) {
// is this how to handle type mismatch?
Copy link
Member

Choose a reason for hiding this comment

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

yes, would say this is the best approach.
alternative would be to throw an exception - this is also done here and there.

but I would say, we should also add some notes to the logs, like here for example

}

if (!$this->attribute) {
// is this how to handle no attrinute
Copy link
Member

Choose a reason for hiding this comment

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

see comment above

$getter = 'get' . ucfirst($this->attribute);

if (!method_exists($inputData, $getter)) {
// is there a better default here?
Copy link
Member

Choose a reason for hiding this comment

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

see comment above

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.

3 participants