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 setOptions method #974

Merged
merged 2 commits into from
Oct 2, 2023
Merged

Fix setOptions method #974

merged 2 commits into from
Oct 2, 2023

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented May 17, 2023

I think it is not necessary to deprecate the setOptions method, the problem was that it removed all the default settings, but with app()->make('dompdf.options') we get a copy of config/dompdf.php before adding the options, the functionality is maintained and it is shorter to use than the original method
https://github.com/dompdf/dompdf/blob/56a660ce045ee27c84154c60b612c936ddb86398/src/Dompdf.php#L1327
Also with an empty array we could reset options to default config/dompdf.php config, example: $pdf->setOptions([])

Closes #793 (comment)

UPDATE: Test added

@erikn69
Copy link
Contributor Author

erikn69 commented Sep 28, 2023

@barryvdh test added

@barryvdh
Copy link
Owner

barryvdh commented Oct 2, 2023

I think this is better indeed, but not sure if this is a breaking change. Although setOptions might not have been usable.

@barryvdh barryvdh merged commit 0918341 into barryvdh:master Oct 2, 2023
24 checks passed
@Suiding
Copy link

Suiding commented Mar 15, 2024

@barryvdh, Depending on the implementation of the config this could have breaking changes. As I've discovered is the case in some of our projects config after updating the dependency to v2.1.0

@barryvdh
Copy link
Owner

How where you using this?

@Suiding
Copy link

Suiding commented Mar 15, 2024

We are using this mainly by calling Barryvdh\DomPDF\Facade\Pdf::setOptions($our_options)->loadView('view', $data) to set just the options that we defined on runtime. Ignoring all defaults from dompdf.options. This worked fine for us.

Right now (v2.1.0) our options get merged with the defaults, which seems to have some values that are not liked by either dompdf/dompdf or dompdf/php-font-lib as this gives us an error to load the desired font after updating the dependency of barryvdh/laravel-dompdf to v2.1.0. Reverting just barryvdh/laravel-dompdf back to v2.0.1 solves the issue.

Thus the functionality of the setOptions() was adjusted in a way, that this is a breaking change.

barryvdh added a commit that referenced this pull request Mar 15, 2024
barryvdh added a commit that referenced this pull request Mar 15, 2024
@barryvdh
Copy link
Owner

Reverted https://github.com/barryvdh/laravel-dompdf/releases/tag/v2.1.1

@Suiding
Copy link

Suiding commented Mar 15, 2024

I'd suggest to make a second optional parameter on the function which will determine if the configs should be merged or not. To maintain backward compatibility I'd suggest something like this.

    public function setOptions(array $options, bool $mergeWithDefaults = false): self
    {
        if ($mergeWithDefaults ) {
            $dompdfOptions = new Options(app()->make('dompdf.options'));
            $dompdfOptions->set($options);
        } else {
            $dompdfOptions = new Options($options);
        }
        $this->dompdf->setOptions($dompdfOptions);
        return $this;
    }

Ah, after posting this reply I see you just reverted the change.

@cesarreyes3
Copy link
Contributor

@barryvdh Now everything fails for me because it resets to options that are not valid, instead of the default options that I defined in my configuration file, it is a breaking change for me

I'd suggest to make a second optional parameter on the function which will determine if the configs should be merged or not. To maintain backward compatibility I'd suggest something like this.

This seems like a better option.

@cesarreyes3 cesarreyes3 mentioned this pull request Mar 15, 2024
@josevv28
Copy link

Now everything fails for me because it resets to options that are not valid, instead of the default options that I defined in my configuration file

+1

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

Successfully merging this pull request may close these issues.

setOptions method breaks image loading
5 participants