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

Restore locales before calling test assertions #45

Merged
merged 5 commits into from
Mar 23, 2021

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Jan 12, 2021

Restore locales before calling assertions otherwise sql will be broken:

From #34 (comment):

seems enabling locale tests by installing needed locales, brings more trouble than needed.

the tests are not run in proper isolation, i.e global resource like locale must be restored after a test, and that is not performed, neither are tests run in isolation (i think phpunit 3.7 does not even support that), and sql queries are written with very bad style, resulting syntax error as numeric value value formatted with comma according to locale rules, gets parsed as two values in sql.

2) Zend_Db_Table_Select_MysqliTest::testSelectWhereOr
Zend_Db_Exception: SQL error for "INSERT INTO `zfprice` (`product_id`, `price_name`, `price_total`) VALUES (1, 'Price 1', 200,45)": Column count doesn't match value count at row 1

NOTE: fixing the DB layer to be independent on locale is a legacy that zf1 carries and can't be fixed.

@glensc glensc force-pushed the gh-actions-locales branch from b77aef4 to 708962a Compare January 12, 2021 18:43
@glensc glensc force-pushed the gh-actions-locales branch from 708962a to 20c8a01 Compare March 13, 2021 16:22
@glensc
Copy link
Contributor Author

glensc commented Mar 13, 2021

#49 has been merged, so can rebase now to see that pdo tests will start to fail because they are not locale aware.

@glensc glensc force-pushed the gh-actions-locales branch from 20c8a01 to 1f84899 Compare March 14, 2021 08:13
@glensc
Copy link
Contributor Author

glensc commented Mar 15, 2021

I'm unable to reproduce the "locales break float and create sql injection' problem.
This should have been the job that failed, but it didn't:

but I guess it's not that important anymore, as the code passes now.

@glensc glensc force-pushed the gh-actions-locales branch from 20211c5 to 862a0a8 Compare March 15, 2021 11:57
@glensc glensc marked this pull request as ready for review March 15, 2021 11:57
@glensc glensc changed the title Enable locale testing in CI Restore locales before calling assertions Mar 15, 2021
@glensc glensc changed the title Restore locales before calling assertions Restore locales before calling test assertions Mar 15, 2021
tests/Zend/Locale/FormatTest.php Outdated Show resolved Hide resolved
tests/Zend/Validate/FloatTest.php Outdated Show resolved Hide resolved
tests/Zend/Validate/FloatTest.php Show resolved Hide resolved
tests/Zend/Locale/FormatTest.php Outdated Show resolved Hide resolved
@glensc glensc marked this pull request as draft March 15, 2021 14:15
@glensc glensc force-pushed the gh-actions-locales branch 2 times, most recently from 9eafbd0 to 75d165c Compare March 15, 2021 14:31
@glensc glensc marked this pull request as ready for review March 15, 2021 14:31
Copy link
Member

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm very glad that you sorted this issue out

tests/Zend/Locale/FormatTest.php Outdated Show resolved Hide resolved
glensc added 4 commits March 15, 2021 22:18
This is to clearly indicate specific encoding is expected
This ensures global state like locale is restored before throwing
exceptions.
@glensc glensc force-pushed the gh-actions-locales branch from 9942b93 to 4d0f809 Compare March 15, 2021 20:18
Copy link
Member

@falkenhawk falkenhawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glensc Is it safe to merge now? It looks good to me.

@glensc
Copy link
Contributor Author

glensc commented Mar 23, 2021

@falkenhawk do not remember. merge if it's ok with you.

@falkenhawk falkenhawk merged commit 616a12e into zf1s:master Mar 23, 2021
@glensc glensc deleted the gh-actions-locales branch March 23, 2021 14:01
@glensc glensc mentioned this pull request Aug 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants