fix: remove content* methods from UrlPdfBuilder and UrlScreenshotBuilder

[marilenaRM]

Q	A
Gotenberg API version ?	8.x
Bug fix ?	yes
New feature ?	no
BC break ?	no
Issues	Fix #257

Description

For URL-based builders, the page body is provided by the URL itself. Exposing content(), contentRaw() and contentFile() was misleading since those methods have no effect on URL conversions.

[Jean-Beru]

Thanks you @marilenaRM !

It could be considered as a BC break but it's a bug fix... Maybe deprecating these methods will be more appropriated. WDYT @Neirda24 @StevenRenaux

PS: don't forget to update UPGRADE file to mention this change.

[StevenRenaux]

Thanks you @marilenaRM !

It could be considered as a BC break but it's a bug fix... Maybe deprecating these methods will be more appropriated. WDYT @Neirda24 @StevenRenaux

PS: don't forget to update UPGRADE file to mention this change.

It would work I would say yes to the BC break but as it is not the case, not sure of the usefulness

[Neirda24]

Shouldn't we split this in a dedicated trait ? As experienced before in this project we noticed that handling trait conflict while using reflection might just get things messy. Not in this specific case though.

Don't mind the BC it is a bugfix to me as it doesn't produce anything I guess.

[marilenaRM]

Hello ! :)

@Jean-Beru : I just added a commit for the update of the file UPGRADE :)
@Neirda24 and @StevenRenaux the BC break notification seems better since we remove some (most certainly not used) public functions, so in my opinion it should be noticed.

@Neirda24 I pushed a separate commit for a way to introduce a separated Trait, WDYT?

[Jean-Beru]

Don't mind the BC it is a bugfix to me as it doesn't produce anything I guess.

That's the reason why it can break current code. The following code works even if the content method is ignored. Removing it will lead to fatal a error.

$builder
    ->content(/*...*/)
    ->url(/* ... */)

Shouldn't we split this in a dedicated trait ?

👍

ContentTrait can be split in 2 traits: content vs. header/footer (a.k.a. marginal ?).

Then we could:

• deprecated content* methods in UrlPdfBuilder
• deprecate header* and footer* methods in ContentTrait and invite to use MarginalTrait instead
• use MarginalTrait in our builder to override the deprecated header* and footer* methods

Later we could:

• remove ContentTrait usage in UrlPdfBuilder and MarkdownPdfBuilder
• remove header* and footer* methods in ContentTrait

[StevenRenaux]

Don't mind the BC it is a bugfix to me as it doesn't produce anything I guess.

That's the reason why it can break current code. The following code works even if the content method is ignored. Removing it will lead to fatal a error.

$builder
    ->content(/*...*/)
    ->url(/* ... */)

Shouldn't we split this in a dedicated trait ?

👍

ContentTrait can be split in 2 traits: content vs. header/footer (a.k.a. marginal ?).

Then we could:

• deprecated content* methods in UrlPdfBuilder
• deprecate header* and footer* methods in ContentTrait and invite to use MarginalTrait instead
• use MarginalTrait in our builder to override the deprecated header* and footer* methods

Later we could:

• remove ContentTrait usage in UrlPdfBuilder and MarkdownPdfBuilder
• remove header* and footer* methods in ContentTrait

This solution LGTM 🤩

[marilenaRM]

so, what's the plan? deprecate in 1.2 and BC Break in 2.0?
maybe we should also address the usage of header and footer in ScreenShots in the referenced issue, deprecate them as well? I'll roll back my changes and split the Traits responsabilities, if that's ok with you

[Jean-Beru]

so, what's the plan? deprecate in 1.2 and BC Break in 2.0? maybe we should also address the usage of header and footer in ScreenShots in the referenced issue, deprecate them as well? I'll roll back my changes and split the Traits responsabilities, if that's ok with you

LGTM