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

Specify data type in html tags using attributes #3445

Merged

Conversation

PouriaSeyfi
Copy link
Contributor

Due to #3444 issue
some times we need specify data type in html using attribute html.
I add new processDomElementDataType function in HTML.php and handle this.

@PouriaSeyfi
Copy link
Contributor Author

There is no any changes. please review my PR.

@PouriaSeyfi PouriaSeyfi closed this Mar 9, 2023
@PouriaSeyfi PouriaSeyfi reopened this Mar 10, 2023
@PouriaSeyfi PouriaSeyfi force-pushed the feature/specify-datatype-in-html branch from 0f3a9ca to eac0aef Compare March 10, 2023 09:19
@PouriaSeyfi
Copy link
Contributor Author

I had some unwanted changes caused by phpstorm formatter. I fixed them and fixup my commits to new one .
Also I update my branch with new changes in master. there is no conflict.
Please Review and merge my changes

@PouriaSeyfi PouriaSeyfi force-pushed the feature/specify-datatype-in-html branch from eac0aef to 6b85c37 Compare March 10, 2023 09:29
@oleibman
Copy link
Collaborator

I think there is probably some merit in this suggestion. I am trying to think through some of its ramifications. Before I start, there is one minor code change that the PR needs - you do not need to call setDataType after setCellValueExplicit.

Calling setCellValueExplicit can result in an Exception, e.g. if you supply an invalid data type. Is that what we want to happen here, or are we better off catching the Exception and ignoring the format? Regardless of that decision, we need some unit tests for this.

Your example for setting the type to formula is intriguing. However, your test case proves that formulas are already handled without the need to explicitly set the data type. What is not handled is treating a string beginning with an equal sign as a string (by setting quotePrefix to true) rather than a formula. You could accommodate this in your change by checking for this condition if data type is supplied as s.

I think that dates are an especially good use case for this change. You should add some examples for type d (ISO date, time, and date/time elements). Note that those will be converted from string to the Excel numeric representation, and that the conversion may fail.

@PouriaSeyfi
Copy link
Contributor Author

PouriaSeyfi commented Mar 17, 2023

thanks for your great explanation.
I think it is better to check datatypes in flushCell function for better performance.
And as you mentioned it is better to catch the Exception and ignore the invalid data types in HTML attributes.
Also, I considered the equal sign at the beginning of cell content in the case of string datatypes. I added some new unit tests for this scenario and iso date datatype.

@PouriaSeyfi PouriaSeyfi force-pushed the feature/specify-datatype-in-html branch 3 times, most recently from 4a91338 to b570d75 Compare March 17, 2023 00:46
@PouriaSeyfi PouriaSeyfi force-pushed the feature/specify-datatype-in-html branch from b570d75 to 9037905 Compare March 17, 2023 00:54
src/PhpSpreadsheet/Reader/Html.php Outdated Show resolved Hide resolved
src/PhpSpreadsheet/Reader/Html.php Outdated Show resolved Hide resolved
tests/PhpSpreadsheetTests/Reader/Html/HtmlTest.php Outdated Show resolved Hide resolved
tests/PhpSpreadsheetTests/Reader/Html/HtmlTest.php Outdated Show resolved Hide resolved
@PouriaSeyfi PouriaSeyfi force-pushed the feature/specify-datatype-in-html branch from 2b43aaa to ceb87b0 Compare March 18, 2023 00:38
@PouriaSeyfi PouriaSeyfi requested review from oleibman and removed request for MarkBaker March 18, 2023 00:38
@PouriaSeyfi PouriaSeyfi force-pushed the feature/specify-datatype-in-html branch from ceb87b0 to 843830f Compare March 18, 2023 06:01
@PouriaSeyfi PouriaSeyfi requested review from MarkBaker and oleibman and removed request for oleibman and MarkBaker March 18, 2023 13:36
@oleibman oleibman merged commit 603d093 into PHPOffice:master Mar 18, 2023
@oleibman
Copy link
Collaborator

Thank you for your contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants