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.

Sign up for GitHub

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

Jump to bottom

Revert #1060 #1066

Merged
merged 1 commit into from Jan 20, 2023
Merged

Revert #1060 #1066

merged 1 commit into from Jan 20, 2023

Conversation

lukebakken
Copy link
Collaborator

Fixes #1065

@lukebakken lukebakken added this to the 3.5.1 milestone Jan 18, 2023
@lukebakken lukebakken self-assigned this Jan 18, 2023
@ramunasd
Copy link
Member

I do not support this revert. We cannot evolve if we support all incorrect library use.

@lukebakken lukebakken force-pushed the lukebakken/partial-revert-1060 branch from 47afa47 to 04aeb5d Compare January 18, 2023 18:48
@lukebakken
Copy link
Collaborator Author

It's alright, we'll fix this for good in 4.0.0 - #1067

@lukebakken lukebakken force-pushed the lukebakken/partial-revert-1060 branch from 04aeb5d to 561d9c0 Compare January 18, 2023 18:55
@lukebakken lukebakken marked this pull request as ready for review January 18, 2023 18:57
@codecov
Copy link

codecov bot commented Jan 18, 2023 •
edited

Codecov Report

Merging #1066 (da998af) into master (71eff31) will increase coverage by 0.40%.
The diff coverage is 90.90%.

@@             Coverage Diff              @@
##             master    #1066      +/-   ##
============================================
+ Coverage     72.92%   73.33%   +0.40%     
- Complexity     1016     1018       +2     
============================================
  Files            39       39              
  Lines          3084     3090       +6     
============================================
+ Hits           2249     2266      +17     
+ Misses          835      824      -11
Impacted Files Coverage Δ
PhpAmqpLib/Wire/IO/StreamIO.php 67.96% <83.33%> (+0.29%) ⬆️
PhpAmqpLib/Connection/AMQPConnectionConfig.php 74.40% <100.00%> (ø)
PhpAmqpLib/Connection/AMQPSSLConnection.php 90.62% <100.00%> (+3.52%) ⬆️
PhpAmqpLib/Connection/AbstractConnection.php 75.63% <0.00%> (+0.46%) ⬆️
...ection/Heartbeat/AbstractSignalHeartbeatSender.php 80.00% <0.00%> (+5.00%) ⬆️
PhpAmqpLib/Wire/IO/AbstractIO.php 92.53% <0.00%> (+11.94%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

ramunasd
ramunasd requested changes Jan 18, 2023
View reviewed changes
Copy link
Member

@ramunasd ramunasd left a comment

Choose a reason for hiding this comment

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

If this revert is temporary until version 4.0 then we should throw some kind of deprecation warning when a non secure "SSL" connection is being created.

@lukebakken
Copy link
Collaborator Author

That's a great idea - how would you prefer to do that? Feel free to push code to my branch, by the way.

@lukebakken
Copy link
Collaborator Author

Looks like E_USER_DEPRECATED and trigger_error.

I will test this out.

@lukebakken lukebakken force-pushed the lukebakken/partial-revert-1060 branch 2 times, most recently from 2dba00c to c2ef556 Compare January 18, 2023 23:34
@lukebakken
Copy link
Collaborator Author

@ramunasd this appears to do the right thing:

https://github.com/php-amqplib/php-amqplib/blob/lukebakken/partial-revert-1060/PhpAmqpLib/Connection/AMQPSSLConnection.php#L30-L33

Calling trigger_error in PHP 7 caused the unit tests to fail, so the behavior of that function must have changed. In the CI output, I only see deprecation log messages in other libraries for PHP 8 and higher so I suspect this is the correct way to do it 🤷

@ramunasd ramunasd force-pushed the lukebakken/partial-revert-1060 branch from 21e0ec3 to 052490f Compare January 19, 2023 08:36
@lukebakken @andrew-demb
Revert #1060
da998af 
Fixes #1065

Add deprecation error

Add non-TLS SSL test

Only trigger_error in version 8 and higher

do not report user deprecations as errors in phpunit

Update PhpAmqpLib/Connection/AMQPSSLConnection.php

Co-authored-by: Andrii Dembitskyi <andrew.dembitskiy@gmail.com>
@lukebakken lukebakken force-pushed the lukebakken/partial-revert-1060 branch from 8be23f1 to da998af Compare January 20, 2023 00:56
@lukebakken lukebakken merged commit bbaccfe into master Jan 20, 2023
@lukebakken lukebakken deleted the lukebakken/partial-revert-1060 branch January 20, 2023 01:00
@LeonMelis LeonMelis mentioned this pull request Jan 20, 2023
Closed
kratkyzobak pushed a commit to kratkyzobak/php-amqplib that referenced this pull request Feb 9, 2024
@lukebakken @andrew-demb
Revert php-amqplib#1060 (php-amqplib#1066)
041c0bb 
Fixes php-amqplib#1065

Add deprecation error

Add non-TLS SSL test

Only trigger_error in version 8 and higher

do not report user deprecations as errors in phpunit

Update PhpAmqpLib/Connection/AMQPSSLConnection.php

Co-authored-by: Andrii Dembitskyi <andrew.dembitskiy@gmail.com>

Co-authored-by: Andrii Dembitskyi <andrew.dembitskiy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrew-demb andrew-demb andrew-demb left review comments

@ramunasd ramunasd ramunasd approved these changes

Assignees

@lukebakken lukebakken

Labels
None yet
Projects
None yet
Milestone
3.5.1
Development

Successfully merging this pull request may close these issues.

Connect error since 3.5.0
3 participants
@lukebakken @ramunasd @andrew-demb