Cell->hasValidValue() - test value on list validation #257

SailorMax · 2017-10-20T07:41:38Z

This is:

a bugfix
a new feature

Checklist:

Changes are covered by unit tests
Code style is respected
Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
CHANGELOG.md contains a short summary of the change
Documentation is updated as necessary

What does it change?
Add possibility to check value of cell on validity based on list of possible values.

PowerKiKi

Thank you for your contribution. It seems like an interesting feature. However we should probably keep it separated from the Cell class to avoid making this class even bigger than it already is. I suggest moving your code to a new, independent class. Maybe something like PhpOffice\PhpSpreadsheet\Cell\Validator ?

PowerKiKi · 2017-10-21T13:42:36Z

src/PhpSpreadsheet/Cell.php

+        // TODO: write check on all cases
+        switch ($cellValidation->getType()) {
+            case Cell\DataValidation::TYPE_LIST:
+                $formula1 = $cellValidation->getFormula1();


We should extract the validation specific for a type into their own method. That would prevent hasValidValue() to become too huge in the future when we add validation for other types.

PowerKiKi · 2017-10-21T13:45:18Z

src/PhpSpreadsheet/Cell.php

+                                $this->getWorksheet()->getParent()
+                            )->calculateFormula($match_formula, $this->getCoordinate(), $this);
+
+                            return $result != '#N/A';


Let's use Functions::NA() instead of hardcoded string and a strict comparison !==.

PowerKiKi · 2017-10-21T13:55:52Z

Oh btw, next time use composer fix to fix code style.

SailorMax · 2017-10-23T07:45:21Z

in PhpOffice\PhpSpreadsheet\Cell\Validator
or in PhpOffice\PhpSpreadsheet\Cell\DataValidation?

as regular method or as static method?

what name for method?

PowerKiKi · 2017-10-23T08:59:53Z

It should definitely be a different class than DataValidation. Maybe DataValidator, if you want to make the relation between the two a bit more obvious. And I'd go for instance methods to prevent pitfall of singleton pattern in future code evolution. So usage could be like: $validator = new DataValidator (); $isValid = $validator->isValid($cell);

PowerKiKi

Thanks, I think it's better that way. But we can improve it even further. Also will have to reach 100% coverage of the class with phpunit. I'll check that later...

PowerKiKi · 2017-10-23T11:41:13Z

src/PhpSpreadsheet/Cell.php

+     *
+     * @return bool
+     */
+    public function hasValidValue()


I think we can drop entirely this method. Keeping the cell class unchanged. This limit the public API to the strict minimum for easier maintenance in the future.

But this is part of cell as object. Isn't it?
Where is border? Which method has to be in the Cell, and which has to work as helper?

Cell has setDataValidation() and hasDataValidation() methods, but hasn't any methods to validate?

@PowerKiKi
So? Do you think? Or do you insist? :)
What is in priority? Maintenance or Usability?

I had another look and the Cell class is not as huge as I though it was, so let's keep the method here as you suggest. But the if (!$this->hasDataValidation()) should still be moved in the validator, otherwise the validation logic would be split into two different places.

moved. But now we have some kind of conflict. GitHub do not allow me even view where is the problem.
Can you fix it?

PowerKiKi · 2017-10-23T11:44:11Z

src/PhpSpreadsheet/Cell/DataValidator.php

+     *
+     * @return bool
+     */
+    public function isValidCellValue(&$cell)


Because there is only one thing that can be validated, the method can simply be "isValid". If you think it might not be clear enough, then we should find a better name for the class, but not the method.

Also the parameter must be type hinted with Cell, and it should not be passed by reference.

PowerKiKi · 2017-10-23T11:48:58Z

src/PhpSpreadsheet/Cell/DataValidator.php

+
+        // TODO: write check on all cases
+        switch ($dataValidation->getType()) {
+            case DataValidation::TYPE_LIST:


Let's refactor the code for list validation in a private method. Something like "isValueInList()" or similar...

PowerKiKi · 2017-10-23T11:49:59Z

src/PhpSpreadsheet/Cell/DataValidator.php

+            case DataValidation::TYPE_LIST:
+                $formula1 = $dataValidation->getFormula1();
+                if (!empty($formula1)) {
+                    if ($formula1[0] == '"') {                         // inline values list


This should use strict equality ===

PowerKiKi · 2017-10-30T08:17:58Z

src/PhpSpreadsheet/Cell.php was moved to src/PhpSpreadsheet/Cell/Cell.php yesterday. So the file you are trying to modify no longer exists. You should rebase your branch onto latest develop and then force push. You can try that by yourself, or else I can give it a try, but not today...

SailorMax · 2017-10-30T11:09:07Z

Not beauty rebase, but done.

Now I trying to make it better in local branch, but don't really understand how to do it. Can you help me? Do I need shift split point to end of develop-branch? How?

SailorMax · 2017-10-30T11:29:51Z

Looks like simple merge is better :)

PowerKiKi · 2017-10-31T05:13:25Z

Thanks !

…dation rules `$cell->hasValidValue()` returns true if the cell has a value which conform to the rules defined in `$cell->getDataValidation()`. Closes PHPOffice#257

SailorMax added 6 commits October 20, 2017 10:38

Cell->hasValidValue() - test value on list validation

07ac3e8

syntax fix

db2ff60

syntax fix

16a2525

syntax fix

e729586

syntax fix

7ce2366

syntax fix

a6e2c10

PowerKiKi requested changes Oct 21, 2017

View reviewed changes

SailorMax added 2 commits October 23, 2017 14:35

move validator code to DataValidator class

bc26ee5

spaces fix

6c26b3a

PowerKiKi requested changes Oct 23, 2017

View reviewed changes

SailorMax added 3 commits October 23, 2017 15:24

DataValidator -> CellDataValidator + extract isValueInList()

bcf92d9

comment to isValueInList()

0bb7ec6

move part of validation to isValid()

4f2d1b1

SailorMax added 2 commits October 30, 2017 13:25

Merge branch 'develop' into cell_has_valid_value_part1

d9fd2f7

fixes after merge

451f577

SailorMax force-pushed the cell_has_valid_value_part1 branch from 766e02d to 451f577 Compare October 30, 2017 11:28

PowerKiKi closed this in 6561494 Oct 31, 2017

SailorMax deleted the cell_has_valid_value_part1 branch March 1, 2019 14:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cell->hasValidValue() - test value on list validation #257

Cell->hasValidValue() - test value on list validation #257

SailorMax commented Oct 20, 2017 •

edited

Loading

PowerKiKi left a comment

PowerKiKi Oct 21, 2017

SailorMax Oct 23, 2017

PowerKiKi Oct 21, 2017

PowerKiKi commented Oct 21, 2017

SailorMax commented Oct 23, 2017 •

edited

Loading

PowerKiKi commented Oct 23, 2017 via email

PowerKiKi left a comment

PowerKiKi Oct 23, 2017

SailorMax Oct 23, 2017 •

edited

Loading

SailorMax Oct 25, 2017

PowerKiKi Oct 29, 2017

SailorMax Oct 30, 2017

PowerKiKi Oct 23, 2017

PowerKiKi Oct 23, 2017

PowerKiKi Oct 23, 2017

PowerKiKi commented Oct 30, 2017

SailorMax commented Oct 30, 2017

SailorMax commented Oct 30, 2017

PowerKiKi commented Oct 31, 2017

Cell->hasValidValue() - test value on list validation #257

Cell->hasValidValue() - test value on list validation #257

Conversation

SailorMax commented Oct 20, 2017 • edited Loading

PowerKiKi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PowerKiKi commented Oct 21, 2017

SailorMax commented Oct 23, 2017 • edited Loading

PowerKiKi commented Oct 23, 2017 via email

PowerKiKi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SailorMax Oct 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PowerKiKi commented Oct 30, 2017

SailorMax commented Oct 30, 2017

SailorMax commented Oct 30, 2017

PowerKiKi commented Oct 31, 2017

SailorMax commented Oct 20, 2017 •

edited

Loading

SailorMax commented Oct 23, 2017 •

edited

Loading

SailorMax Oct 23, 2017 •

edited

Loading