Add support for PHP 8 #197

ben221199 · 2021-01-22T10:02:17Z

This PR adds support for PHP 8.

Get multiple facets in single query to work

ben221199 · 2021-01-22T10:31:29Z

Seems Travis is hanging.

composer.json

mfjordvald · 2021-01-31T22:45:53Z

Would be great to get this merged in.

ben221199 · 2021-02-03T14:31:55Z

@Gemorroj Changes are made. Where are we waiting for?

Gemorroj · 2021-02-03T14:35:36Z

@ben221199 im not maintainer of that project) im waiting the change like you

ben221199 · 2021-02-03T14:51:53Z

@ben221199 im not maintainer of that project) im waiting the change like you

Ah okay.
@oohnoitz I see that you did merge PRs in the past. Could you also check this one?

woxxy · 2021-02-03T15:13:51Z

We need to get the CI to work first, will look into it when I have some time

CHANGELOG.md

oohnoitz · 2021-02-03T15:25:17Z

just had a tiny nit regarding the CHANGELOG modifications. but we'll need to look into the CI as @woxxy mentioned.

ben221199 · 2021-02-05T23:04:01Z

The last version was 2.1.0, so the next version could be 2.1.1, 2.2.0 or 3.0.0. Because we add support for PHP 8, I would say the best choice is to go to 3.0.0. I also used that version in the Change Log.

oohnoitz · 2021-02-07T22:54:59Z

Next version would be 3.0.0 since renaming would be a breaking change. Also, don't see the change pushed up yet.

ben221199 · 2021-02-08T14:52:05Z

Well, that was stupid :D.
I pushed it now.

oohnoitz

there are a few nits regarding code style inconsistency. also, can you fix the travis builds so it passes?

tests/SphinxQL/TestUtil.php

tests/SphinxQL/SphinxQLTest.php

tests/SphinxQL/ConnectionTest.php

tests/SphinxQL/HelperTest.php

ben221199 · 2021-02-09T08:41:53Z

there are a few nits regarding code style inconsistency. also, can you fix the travis builds so it passes?

Is there also a reason why I cannot see the Travis CI build?

ben221199 · 2021-02-09T09:35:17Z

there are a few nits regarding code style inconsistency. also, can you fix the travis builds so it passes?

Is there also a reason why I cannot see the Travis CI build?

Never mind. This was an issue in my browser.

ben221199

Perfect.

scrutinizer-notifier · 2021-02-15T22:15:47Z

The inspection completed: 117 updated code elements

ben221199 · 2021-02-15T22:25:37Z

@oohnoitz Is there anything possible at the moment?

oohnoitz

it's taking me some time to determine what actual changes were made and review them. i'm seeing a few tests that are commented out. is the plan to fix them in a subsequent PR? if that is the case, we can get the current work merged into a branch on this repo and have some follow up PRs done to fix the broken tests. otherwise, i can't go ahead and merge it in as-is since this is removing tests.

oohnoitz

leaving a few comments to track down missing/removed tests. and determine what went wrong. adding php8 support should have caused those tests to break at all.

oohnoitz · 2021-02-16T00:58:09Z

src/Drivers/ResultSet.php

-            $this->stored = $this->affected_rows;
-        } else {
+        if (!$this->adapter->isDml()) {
+//            $this->stored = $this->affected_rows;


any reason this code was commented out?

oohnoitz · 2021-02-16T00:58:59Z

tests/README.md

-There's a sphinx.conf file in this directory. It uses a single RT index. Check the necessary directories, I ran Sphinx in `/usr/local/sphinx`
+Choose the version you want to use (e.g. Sphinx 2, Sphinx 3 or Manticore).
+Then set an environment variable:
+ - Sphinx 2: Typ `SEARCH_BUILD=SPHINX2` in the console


nit: type is misspelled in this file.

oohnoitz · 2021-02-16T00:59:39Z

tests/Drivers/ConnectionTest.php

+//     * @throws SphinxQLException
+//     * @throws DatabaseException
+//     */
+//    public function testMultiQuery(): void


MultiQuery test was removed for some reason?

oohnoitz · 2021-02-16T01:00:20Z

tests/Drivers/MultiResultSetTest.php

+class MultiResultSetTest extends TestCase
+{
+
+//    /**


MultiResultSet test was removed for some reason?

oohnoitz · 2021-02-16T01:02:10Z

tests/FacetTest.php

+class FacetTest extends TestCase
+{
+
+//	public static $DATA = [


this can be removed if it isn't used at all in the test file.

oohnoitz · 2021-02-16T01:02:48Z

tests/HelperTest.php

+    //	 */
+    //	public function testCreateFunction(): void
+    //	{
+    //		$this->createHelper()->createFunction('my_udf', 'INT', 'test_udf.so')->execute();


createFunction test was removed for some reason?

oohnoitz · 2021-02-16T01:03:13Z

tests/PercolateTest.php

+class PercolateTest extends TestCase
+{
+
+//    /**


Manticore Percolate test file was removed for some reason?

oohnoitz · 2021-02-16T01:03:44Z

tests/SphinxQLTest.php

+    //	 * @throws DatabaseException
+    //	 * @throws SphinxQLException
+    //	 */
+    //	public function testUpdate(): void


update tests were removed for some reason.

oohnoitz · 2021-02-16T01:04:42Z

tests/SphinxQLTest.php

+//        $this->assertEquals('10', $result[0][0]['id'] ?? null);
+//        $this->assertEquals('1', $result[1][0]['Value'] ?? null);
+//        $this->assertEquals('11', $result[2][0]['id'] ?? null);


what's happening here? it should default to null

oohnoitz · 2021-02-16T01:05:18Z

tests/SphinxQLTest.php

-            $this->assertEquals('3', $result[1][2]['count(*)']);
-            $this->assertEquals('2', $result[1][3]['count(*)']);
+        foreach ([self::$connection, null] as $connection) {
+//            $result = $this->createSphinxQL()


facet tests were removed for some reason? also not sure why we are returning early for Mysqli driver.

ben221199 · 2021-02-16T10:10:34Z

it's taking me some time to determine what actual changes were made and review them. i'm seeing a few tests that are commented out. is the plan to fix them in a subsequent PR? if that is the case, we can get the current work merged into a branch on this repo and have some follow up PRs done to fix the broken tests. otherwise, i can't go ahead and merge it in as-is since this is removing tests.

I think that is the easiest thing to do now.

oohnoitz · 2021-02-16T17:02:40Z

@ben221199 merged it into php8 branch so that it can be worked on there. we'll want to have the remaining tests fixed and added back in.

ben221199 · 2021-02-17T09:49:05Z

Nice. I will try to work on that one. But if other people feel free to help, they are welcome :D

red-led · 2021-04-08T15:42:35Z

Tried updating to php8 branch in my code (composer require foolz/sphinxql-query-builder dev-php8) and getting this error:

mysqli_sql_exception: Commands out of sync; you can't run this command now .../foolz/sphinxql-query-builder/src/Drivers/Mysqli/Connection.php:151
Stack trace:
#0 .../foolz/sphinxql-query-builder/src/Drivers/Mysqli/Connection.php(151): mysqli->multi_query()
#1 .../foolz/sphinxql-query-builder/src/SphinxQL.php(296): Foolz\SphinxQL\Drivers\Mysqli\Connection->multiQuery()
...

Is it bug or BC?

If I just manually rename Match to MathBuilder in old version - it works on php8 without any problems.

ben221199 · 2021-04-08T17:09:19Z

@red-led What version do you use? Sphinx 2, Sphinx 3 or MantiCore?

red-led · 2021-04-08T17:11:42Z

@ben221199, Sphinx 2

ben221199 · 2021-04-09T21:51:16Z

Did you try Sphinx 3?

ben221199 · 2021-04-11T15:32:45Z

Update:
See #198

Jon78 and others added 4 commits May 6, 2020 14:52

Update Facet.php

cd3d9d5

Get multiple facets in single query to work

Update composer file

c4edb56

Rename Match to MatchBuilder

9be0943

Remove setting empty facet array

0d97bb4

ben221199 mentioned this pull request Jan 22, 2021

php 8 support #193

Closed

Fix tests

479f7db

Gemorroj reviewed Jan 31, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

Gemorroj reviewed Jan 31, 2021

View reviewed changes

composer.json Outdated Show resolved Hide resolved

Replace | with || in composer.json

e32732b

woxxy self-assigned this Feb 3, 2021

oohnoitz requested changes Feb 3, 2021

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Fix ChangeLog

a00b702

ben221199 requested a review from oohnoitz February 4, 2021 08:56

oohnoitz requested changes Feb 9, 2021

View reviewed changes

tests/SphinxQL/TestUtil.php Outdated Show resolved Hide resolved

tests/SphinxQL/SphinxQLTest.php Outdated Show resolved Hide resolved

tests/SphinxQL/ConnectionTest.php Outdated Show resolved Hide resolved

tests/SphinxQL/HelperTest.php Outdated Show resolved Hide resolved

ben221199 added 4 commits February 9, 2021 09:56

Fix style

8acb518

Fix issue

2cc14da

Fix test queries

bf2352f

Fix compile function

6efd009

ben221199 added 5 commits February 13, 2021 11:03

Make Shell files executable

0937428

Add C++ source

802bacb

Add configuration and debug statements

6769074

Change location of data folder

6fa5a1f

Fix tests

7aaa1be

ben221199 commented Feb 13, 2021

View reviewed changes

ben221199 added 10 commits February 13, 2021 12:33

Add README file for testing

ee22c72

Export enviroment variables

106dc77

Fix running with environment variables

9bb7bc2

Fix permissions

c793bad

Fix Sphinx3 tests

9ba55ee

Add missing test files

f0ad92e

Fix values function

e238ec8

Fix tests for MySQLi

0979ae9

Fix test style

be97fcb

Remove unused comments

9fbd6cb

oohnoitz reviewed Feb 16, 2021

View reviewed changes

oohnoitz changed the base branch from master to php8 February 16, 2021 17:01

oohnoitz merged commit 73a8b5c into FoolCode:php8 Feb 16, 2021

Add support for PHP 8 #197

Add support for PHP 8 #197

Uh oh!

Conversation

ben221199 commented Jan 22, 2021

Uh oh!

ben221199 commented Jan 22, 2021

Uh oh!

Uh oh!

Uh oh!

mfjordvald commented Jan 31, 2021

Uh oh!

ben221199 commented Feb 3, 2021

Uh oh!

Gemorroj commented Feb 3, 2021

Uh oh!

ben221199 commented Feb 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

woxxy commented Feb 3, 2021

Uh oh!

Uh oh!

oohnoitz commented Feb 3, 2021

Uh oh!

ben221199 commented Feb 5, 2021

Uh oh!

oohnoitz commented Feb 7, 2021

Uh oh!

ben221199 commented Feb 8, 2021

Uh oh!

oohnoitz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ben221199 commented Feb 9, 2021

Uh oh!

ben221199 commented Feb 9, 2021

Uh oh!

ben221199 left a comment

Choose a reason for hiding this comment

Uh oh!

scrutinizer-notifier commented Feb 15, 2021

Uh oh!

ben221199 commented Feb 15, 2021

Uh oh!

oohnoitz left a comment

Choose a reason for hiding this comment

Uh oh!

oohnoitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ben221199 commented Feb 16, 2021

Uh oh!

oohnoitz commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ben221199 commented Feb 17, 2021

ben221199 commented Feb 3, 2021 •

edited

Loading

oohnoitz commented Feb 16, 2021 •

edited

Loading