Skip to content

Fix some PHP 7.3 errors#12718

Merged
markstory merged 6 commits into
cakephp:2.xfrom
bancer:php-7.3
Nov 13, 2018
Merged

Fix some PHP 7.3 errors#12718
markstory merged 6 commits into
cakephp:2.xfrom
bancer:php-7.3

Conversation

@bancer

@bancer bancer commented Nov 11, 2018

Copy link
Copy Markdown
Contributor

No description provided.

@dereuromark

dereuromark commented Nov 11, 2018

Copy link
Copy Markdown
Member

Looks like only 3 errors left to undo the allow failure for php7.3 in travis matrix.

@dereuromark dereuromark added this to the 2.10.14 milestone Nov 11, 2018
garas
garas reviewed Nov 11, 2018
View reviewed changes
Comment thread lib/Cake/Test/Case/Console/Command/Task/ExtractTaskTest.php Outdated
@garas

garas commented Nov 11, 2018

Copy link
Copy Markdown
Member

PHP Warning: PHP Startup: Unable to load dynamic library 'memcached.so'

Disable memcached like in 3.x

cakephp/.travis.yml

Lines 57 to 60 in d080e1d

- |
if [[ ${TRAVIS_PHP_VERSION} != "7.3" && ${TRAVIS_PHP_VERSION} != "5.6" && ($DEFAULT = 1 || $PHPSTAN = 1) ]]; then
echo 'extension = memcached.so' >> ~/.phpenv/versions/$(phpenv version-name)/etc/php.ini;
fi

@dereuromark

dereuromark commented Nov 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Member 
There were 2 failures:
1) MultibyteTest::testUsingMbStrtoupper
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ḀḂḄḆḈḊḌḎḐḒḔḖḘḚḜḞḠḢḤḦḨḪḬḮḰḲḴḶḸḺḼḾṀṂṄṆṈṊṌṎṐṒṔṖṘṚṜṞṠṢṤṦṨṪṬṮṰṲṴṶṸṺṼṾẀẂẄẆẈẊẌẎẐẒẔẖẗẘẙẚẠẢẤẦẨẪẬẮẰẲẴẶẸẺẼẾỀỂỄỆỈỊỌỎỐỒỔỖỘỚỜỞỠỢỤỦỨỪỬỮỰỲỴỶỸ'
+'ḀḂḄḆḈḊḌḎḐḒḔḖḘḚḜḞḠḢḤḦḨḪḬḮḰḲḴḶḸḺḼḾṀṂṄṆṈṊṌṎṐṒṔṖṘṚṜṞṠṢṤṦṨṪṬṮṰṲṴṶṸṺṼṾẀẂẄẆẈẊẌẎẐẒẔH̱T̈W̊Y̊AʾẠẢẤẦẨẪẬẮẰẲẴẶẸẺẼẾỀỂỄỆỈỊỌỎỐỒỔỖỘỚỜỞỠỢỤỦỨỪỬỮỰỲỴỶỸ'
/home/travis/build/cakephp/cakephp/lib/Cake/Test/Case/I18n/MultibyteTest.php:7634
/home/travis/build/cakephp/cakephp/lib/Cake/TestSuite/CakeTestCase.php:84
/home/travis/build/cakephp/cakephp/lib/Cake/TestSuite/CakeTestRunner.php:75
/home/travis/build/cakephp/cakephp/lib/Cake/TestSuite/CakeTestSuiteCommand.php:98
/home/travis/build/cakephp/cakephp/lib/Cake/Console/Command/TestShell.php:283
/home/travis/build/cakephp/cakephp/lib/Cake/Console/Command/TestShell.php:268
/home/travis/build/cakephp/cakephp/lib/Cake/Console/Shell.php:462
/home/travis/build/cakephp/cakephp/lib/Cake/Console/ShellDispatcher.php:219
/home/travis/build/cakephp/cakephp/lib/Cake/Console/ShellDispatcher.php:66
2) MultibyteTest::testUsingMbStrtoupperArmenian
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'ԱԲԳԴԵԶԷԸԹԺԻԼԽԾԿՀՁՂՃՄՅՆՇՈՉՊՋՌՍՎՏՐՑՒՓՔՕՖև'
+'ԱԲԳԴԵԶԷԸԹԺԻԼԽԾԿՀՁՂՃՄՅՆՇՈՉՊՋՌՍՎՏՐՑՒՓՔՕՖԵՒ'
/home/travis/build/cakephp/cakephp/lib/Cake/Test/Case/I18n/MultibyteTest.php:7656
/home/travis/build/cakephp/cakephp/lib/Cake/TestSuite/CakeTestCase.php:84
/home/travis/build/cakephp/cakephp/lib/Cake/TestSuite/CakeTestRunner.php:75
/home/travis/build/cakephp/cakephp/lib/Cake/TestSuite/CakeTestSuiteCommand.php:98
/home/travis/build/cakephp/cakephp/lib/Cake/Console/Command/TestShell.php:283
/home/travis/build/cakephp/cakephp/lib/Cake/Console/Command/TestShell.php:268
/home/travis/build/cakephp/cakephp/lib/Cake/Console/Shell.php:462
/home/travis/build/cakephp/cakephp/lib/Cake/Console/ShellDispatcher.php:219
/home/travis/build/cakephp/cakephp/lib/Cake/Console/ShellDispatcher.php:66

left

@bancer

bancer commented Nov 11, 2018

Copy link
Copy Markdown
Contributor Author

It is not clear why there are tests that assert the results of PHP functions such as mb_strtoupper. I see no point in that. Or should multibyte extension be removed on travis to test the framework's implementation of mb_strtoupper?

@markstory

markstory commented Nov 12, 2018

Copy link
Copy Markdown
Member

Looks pretty good to me. The multi byte class tests are a relic of how old 2.x is. Back when 2.x was started the mbstring extension was not commonly available. From what I remember the tests should be at least partially covering our code and not the just the php extension.

@bancer

bancer commented Nov 12, 2018

Copy link
Copy Markdown
Contributor Author

Some tests like testMultibyteStrtoupper() that use Multibyte class cover CakePHP code. Some tests like testUsingMbStrtoupper() that use mbstring extension methods do not cover CakePHP code when this extension is installed.

@bancer

bancer commented Nov 12, 2018

Copy link
Copy Markdown
Contributor Author

It seems there are some changes in mbstring in PHP 7.3 that affect these tests.

@markstory markstory merged commit 8573c15 into cakephp:2.x Nov 13, 2018
@tenkoma tenkoma mentioned this pull request Apr 28, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garas garas garas left review comments

Assignees

No one assigned

Labels

enhancement

Projects

None yet

Milestone

2.10.14

Development

Successfully merging this pull request may close these issues.

4 participants

@bancer @dereuromark @garas @markstory