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

Make sure to support 8.1 #209

Merged
merged 3 commits into from
Feb 26, 2022
Merged

Make sure to support 8.1 #209

merged 3 commits into from
Feb 26, 2022

Conversation

harikt
Copy link
Member

@harikt harikt commented Feb 26, 2022

Trying to address #194 , #208 .

Some credits for #204

@kenjis
Copy link
Member

kenjis commented Feb 26, 2022

I ran tests on PHP 8.1. Is it okay?

$ vendor/bin/phpunit 
PHPUnit 9.5.16 by Sebastian Bergmann and contributors.

  Warning - The configuration file did not pass validation!
  The following problems have been detected:

  Line 3:
  - Element 'testsuite': The attribute 'name' is required but missing.

  Line 7:
  - Element 'filter': This element is not expected.

  Test results may not be as expected.

@harikt
Copy link
Member Author

harikt commented Feb 26, 2022

Hey @kenjis ,

That was phpunit configuration issue. I have changed it now. The tests are running on 8.1, you can see in github actions.

@kenjis
Copy link
Member

kenjis commented Feb 26, 2022

@harikt Okay, I've confirmed the warnings are gone.

@harikt harikt merged commit 2fe8ad3 into auraphp:3.x Feb 26, 2022
@@ -105,6 +106,9 @@ public function testConnectionQueries()

// make sure new encoding was honored
$actual = $pdo->fetchValue('PRAGMA encoding');

// When changing to UTF-16, sometimes it is returning UTF-16le
// $this->assertEquals($newEncoding, $actual);
Copy link
Member Author

Choose a reason for hiding this comment

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

Can anyone of you run the test uncommenting the line ?

This is what I get

image

Copy link
Member

Choose a reason for hiding this comment

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

$ vendor/bin/phpunit tests/ExtendedPdoTest.php
PHPUnit 9.5.16 by Sebastian Bergmann and contributors.

.F............................................                    46 / 46 (100%)

Time: 00:00.090, Memory: 6.00 MB

There was 1 failure:

1) Aura\Sql\ExtendedPdoTest::testConnectionQueries
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'UTF-16'
+'UTF-16le'

@harikt harikt deleted the fix3x-81 branch February 26, 2022 12:12
@harikt
Copy link
Member Author

harikt commented Feb 26, 2022

Someone thought I am disrespectful to them. YOURLS/YOURLS#3230 (comment) . I am not sure if I did something wrong helping the project.

Let me step back.

@kenjis
Copy link
Member

kenjis commented Feb 26, 2022

It seems they were mad because they thought their PR was closed without any explanation.

@LeoColomb
Copy link

Yes, I'm not mad about the PR itself, but more about the way it has been managed.
Might be a misunderstanding, if so I'm sorry about that.

Yes indeed, my PR has been closed without any explanation, and yes this is contributor harmful.
Usually I don't care, but as this PR was requested, it becomes very touchy.

Also @harikt, be careful with who helps who. We never asked for any help on the mentioned project, YOURLS.
You have commented a change in YOURLS to gather details (which is perfectly fine, glad to help), then you asked for help on your project, Aura.Sql. Not the same!

@harikt
Copy link
Member Author

harikt commented Feb 26, 2022

Hey @LeoColomb ,

Firstly and lastly I apologize if you felt I was rude. Normally I used to wait for the changes so that people get credits for what they deserve.

Today I noticed a closed PR. I thought this was closed by you. Again I didn't closed it myself. If so it may have happened by an accident.

If you got an email stating harikt closed the issue, can you post a screenshot?

I am also curious to know what really happened.

@kenjis
Copy link
Member

kenjis commented Feb 26, 2022

Normally it shows who and when closed:
Screenshot 2022-02-27 7 52 55

But the PR's closed comment is different:
Screenshot 2022-02-27 7 52 42

@LeoColomb
Copy link

LeoColomb commented Feb 27, 2022

Indeed, I didn't received any email about the closure and more importantly my fork does not exist anymore for no reason.
Such an embarrassing situation...
I've sent GitHub support a message to request an investigation.

So, should I recreate my PR?

@harikt
Copy link
Member Author

harikt commented Feb 27, 2022

So, should I recreate my PR?

No. This has been addressed currently.

Thank you for your time.

@koriym
Copy link
Member

koriym commented Feb 27, 2022

I was just about to request GitHub support too!

@pmjones
Copy link
Member

pmjones commented Feb 27, 2022

This is very weird.

@LeoColomb I know how it is when someone has been rude on the internet -- it makes you feel frustrated and angry and you want to push back in return. But if you will take my word for it, @harikt is one of the most polite and respectful people I know, so any sense of rudeness was sure to be a miscommunication.

Thank you @LeoColomb for your patience and understanding.

@koriym
Copy link
Member

koriym commented Feb 28, 2022

Support responded.

Pull requests can sometimes close automatically due to related events, rather than being closed directly by a user. For example, if the pull request originates from a fork, and the fork is then deleted, the associated pull request will be closed automatically. This is because the branch in the pull request no longer exists, having been deleted as part of the fork.

It's not like any person closed the PR directly.

@harikt
Copy link
Member Author

harikt commented Feb 28, 2022

Thank you @pmjones , @koriym , @kenjis for taking your valuable time and trying to resolve the misunderstanding happened due to an event with github. @LeoColomb lets forget the bad stuffs and move on.

Have a nice day ahead.

With love

Hari

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.

5 participants