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

Fix some PHP 7.3 errors #12718

Merged
merged 6 commits into from Nov 13, 2018

Conversation

Projects
None yet
4 participants
@bancer
Contributor

bancer commented Nov 11, 2018

No description provided.

@dereuromark

This comment has been minimized.

Member

dereuromark commented Nov 11, 2018

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

This comment has been minimized.

Member

garas commented Nov 11, 2018

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

bancer added some commits Nov 11, 2018

@dereuromark

This comment has been minimized.

Member

dereuromark commented Nov 11, 2018

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

This comment has been minimized.

Contributor

bancer commented Nov 11, 2018

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

This comment has been minimized.

Member

markstory commented Nov 12, 2018

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

This comment has been minimized.

Contributor

bancer commented Nov 12, 2018

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

This comment has been minimized.

Contributor

bancer commented Nov 12, 2018

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

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment