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

[Task]: Remove unecessary if condition when setting margins via Gotenberg #34

Merged
merged 2 commits into from
Jun 19, 2023

Conversation

matthiashamacher
Copy link
Contributor

If you would set any of the margins, except marginRight to 0 the if condition would evaluate to false and therefore the margins won't be set. By checking all with isset the condition would be true if any one of them is set and the margins will be applied

Signed-off-by: Matthias Hamacher <m.hamacher@basecom.de>
@kingjia90 kingjia90 added this to the 1.0.1 milestone Jun 19, 2023
@kingjia90 kingjia90 added the Bug label Jun 19, 2023
@kingjia90
Copy link
Contributor

kingjia90 commented Jun 19, 2023

Probably we don't need this IF at all because of

$chromium->margins(
$params['marginTop'] ?? 0.39,
$params['marginBottom'] ?? 0.39,
$params['marginLeft'] ?? 0.39,
$params['marginRight'] ?? 0.39
);

If margins() method is not called here, it's going to be 0.39 by default anyways, so this null coalescing would cover it

Signed-off-by: Matthias Hamacher <m.hamacher@basecom.de>
@matthiashamacher
Copy link
Contributor Author

matthiashamacher commented Jun 19, 2023

@kingjia90
Good Point. I've updated the PR and removed the if condition.

@kingjia90 kingjia90 merged commit d620fa3 into pimcore:1.x Jun 19, 2023
6 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jun 19, 2023
@kingjia90 kingjia90 changed the title Check all with isset as 0 would evaluate to false [Task]: Remove unecessary if condition when setting margins via Gotenberg Jun 19, 2023
@kingjia90 kingjia90 added Task and removed Bug labels Jun 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants