trim cells by default #4050
trim cells by default #4050
Conversation
|
How would you propose making this opt in? |
|
Ideally I'd like to add some form of Middleware but it would be too big a refactor. I think either a static property on Row or other class or something that can be configured like the chunkSize. |
|
Isn't this something that could already be implemented with a value binder? |
|
@patrickbrouwers I didn't see the Value Binder was also called in the constructor of a cell to bind the value. i thought it was only called when setting a Cell, not when loading a new spreadsheet. I think we can close this PR indeed |
|
I've been doing some testing though but i don't see the CustomValue binder being hit on import? actually the ValueBinder can only be used when Excell fields have no data types in them: |
|
@patrickbrouwers what would you advise to make this feature work? it would be great to have an option for this! as most people using Excels don't understand whitespaces in their cells and often make this mistake, it would be great to trim them by import by default. 🚀 |
|
@patrickbrouwers can i do something to land this change? |
| @@ -97,7 +99,9 @@ public function toArray($nullValue = null, $calculateFormulas = false, $formatDa | |||
| $i = 0; | |||
| foreach ($this->row->getCellIterator('A', $endColumn) as $cell) { | |||
| $value = (new Cell($cell))->getValue($nullValue, $calculateFormulas, $formatData); | |||
|
|
|||
| if (self::$trimStrings) { | |||
There was a problem hiding this comment.
I would prefer this logic to be within the Cell class https://github.com/SpartnerNL/Laravel-Excel/blob/3.1/src/Cell.php
Instead of a public property, I would prefer a method like: Cell::trimStrings() with a default value of true.
| private function cleanValue($value) | ||
| { | ||
| if(!is_string($value)){ | ||
|
|
There was a problem hiding this comment.
Whiteline can be removed here
| if(!is_string($value)){ | ||
|
|
||
| return $value; | ||
| } |
|
|
||
| return $value; | ||
| } | ||
| $cleaned = preg_replace('~^[\s\x{FEFF}\x{200B}]+|[\s\x{FEFF}\x{200B}]+$~u', '', $value) ?? trim($value); |
There was a problem hiding this comment.
Please add a comment what this preg_replace does.
Also I think this should be ?: instead of ??
| @@ -173,4 +177,19 @@ public function setPreparationCallback(Closure $preparationCallback = null) | |||
| { | |||
| $this->preparationCallback = $preparationCallback; | |||
| } | |||
|
|
|||
| private function cleanValue($value) | |||
|
Please also add a (feature) test that shows that this feature is actually working like expected when enabling it. I would prefer the middleware option via the excel.php config, but I understand that will be more work. I'm okay with having a static method on the Cell class to enable trimming cell values. |
|
Replaced this PR by a middleware implementation that can be found in the config: https://github.com/SpartnerNL/Laravel-Excel/blob/3.1/config/excel.php#L161 |
|
@patrickbrouwers thanks, valid point, didnt see it existed. |
|
No worries, I added it last week |
Please take note of our contributing guidelines: https://docs.laravel-excel.com/3.1/getting-started/contributing.html
Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
1️⃣ Why should it be added? What are the benefits of this change?
see my suggestion here: #4047
2️⃣ Does it contain multiple, unrelated changes? Please separate the PRs out.
no, although i think i should make it backwards compatible with an option or static option method somehwere
3️⃣ Does it include tests, if possible?
not yet.
4️⃣ Any drawbacks? Possible breaking changes?
now yes, but not if i add it behind an option flag?
5️⃣ Mark the following tasks as done:
6️⃣ Thanks for contributing! 🙌