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

Style/Cell.php does not accept null values to unset some values #2673

Closed
2 tasks
SpraxDev opened this issue Sep 11, 2024 · 1 comment · Fixed by #2676
Closed
2 tasks

Style/Cell.php does not accept null values to unset some values #2673

SpraxDev opened this issue Sep 11, 2024 · 1 comment · Fixed by #2676
Milestone

Comments

@SpraxDev
Copy link
Contributor

Describe the bug and add attachments

Some values like vMerge and vAlign are type-hinted as string (not nullable) although they are initialized as null until explicitly set/parsed.

That means, there is no proper way to unset some values where it would make sense to 'remove' that attribute.
The parameter for setVMerge and setVAlign for example even has a default value of null set, which is not valid according to its type-hint and makes everything even more confusing.

Expected behavior

I think it would make sense to allow null to be set for some attributes and everything to be type-hinted accordingly.

Steps to reproduce

$phpWord = new PhpWord();
$section = $phpWord->addSection();

$table = $section->addTable();
$row = $table->addRow();

$cell = $row->addCell();
$cell->addText('no vAlign');
$cell->getStyle()->setVAlign(VerticalJc::BOTTOM);
$cell->getStyle()->setVAlign();

PHPWord version(s) where the bug happened

master

PHP version(s) where the bug happened

8.3 and all the others

Priority

  • I want to crowdfund the bug fix (with @algora-io) and fund a community developer.
  • I want to pay the bug fix and fund a maintainer for that. (Contact @Progi1984)
@SpraxDev
Copy link
Contributor Author

SpraxDev commented Sep 11, 2024

I wouldn't mind creating a PR for that if allowing null is the correct thing to do.
I'm planning to do some PRs anyway already – I just wasn't sure if such a PR would be accepted or whether another solution would be preferred.

SpraxDev added a commit to SpraxDev/PHPWord that referenced this issue Sep 12, 2024
…ce#2673)

vAlign and vMerge are initialized as `null` in every style until it is explicitly set.
Right now, it is not possible to unset them, after it has been set once.

I've added a null-check to skip the validation, based on the default
parameter value of `null`, which indicates to me that it once was intended to work like this.

I've also fixed the type-hints, which were wrong from the start.
SpraxDev added a commit to SpraxDev/PHPWord that referenced this issue Sep 13, 2024
…ce#2673)

vAlign and vMerge are initialized as `null` in every style until it is explicitly set.
Right now, it is not possible to unset them, after it has been set once.

I've added a null-check to skip the validation, based on the default
parameter value of `null`, which indicates to me that it once was intended to work like this.

I've also fixed the type-hints, which were wrong from the start.
Progi1984 pushed a commit that referenced this issue Sep 13, 2024
* fix: Allow vAlign and vMerge on Style\Cell to be set to null (#2673)

vAlign and vMerge are initialized as `null` in every style until it is explicitly set.
Right now, it is not possible to unset them, after it has been set once.

I've added a null-check to skip the validation, based on the default
parameter value of `null`, which indicates to me that it once was intended to work like this.

I've also fixed the type-hints, which were wrong from the start.

* docs(changelog): Add note about supporting vAlign and vMerge to be unset

* test: Extend existing table test case to check if unsetting vAlign works

* test: Add test case to check if unsetting vMerge works

This should fix the reduction in test coverage because of the
new null checking code in the Setter
@Progi1984 Progi1984 added this to the 1.4.0 milestone Sep 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

2 participants