barryvdh / laravel-dompdf

A DOMPDF Wrapper for Laravel
MIT License
6.62k stars 965 forks source link

Fix setOptions method #974

Closed erikn69 closed 11 months ago

erikn69 commented 1 year ago

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 https://github.com/barryvdh/laravel-dompdf/issues/793#issuecomment-866519800

UPDATE: Test added

erikn69 commented 11 months ago

@barryvdh test added

barryvdh commented 11 months ago

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

Suiding commented 5 months ago

@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 commented 5 months ago

How where you using this?

Suiding commented 5 months ago

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 commented 5 months ago

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

Suiding commented 5 months ago

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 commented 5 months ago

@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.

josevv28 commented 5 months ago

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