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

Fix/refactor creation of RabbitMQ Connection and QueueAPI #528

Merged
merged 27 commits into from
Apr 11, 2023
Merged

Fix/refactor creation of RabbitMQ Connection and QueueAPI #528

merged 27 commits into from
Apr 11, 2023

Conversation

adm-bome
Copy link
Collaborator

@adm-bome adm-bome commented Mar 11, 2023 •
edited
Loading

Refactor:

Creating Connection with config via a factory.
Creating QueueAPI with config via a factory.

Prepaired the library for deprication of the AMQPLazyConnection:class

This is an Minor update
The code respects the old way connection and QueueApi were created. In a Major version some code can be removed when decided not supporting the old depricated AMQPLazyConnection::class

The previous versions where not realy Lazy. As soon as the QueueApi class (RabbitMQQueue::class) was created a channel was opened up. ( in the contruction) and when that happens the connection to RabbitMQ was open or "connected".
This version builds all classes but only opens the connection when a channel is needed (push, pop....)

Maybe this refactor also will help with the Octane problem of a connection being closed by RabbitMQ. (don´t no for sure.) but Lazy should be lazy 😉

#513, #521, #527, #487 and #460

adm-bome added 12 commits March 2, 2023 19:09
@adm-bome
added:
f2f7a5e 
  - factory for making a queue.
  - default config object
  - rabbitmq queue contract

fixed:
  - code quality issues
  - tests (for checking moved config items)

removed:
  - unnecessary config
  - unnecessary methods for returning just the config
@adm-bome
fixed: static factory call
3a32d71
@adm-bome
fixed:
019a411 
 - reverted config tests
 - private config getter
@adm-bome
fixed:
aee2a42 
 - phpdoc tags
@adm-bome
added:
3cee3ff 
  - factory for making a connection.
  - factory for creating a config object.
  - test lazy stream connection.
fixed:
  - tests

removed:
  - unnecessary methods
@adm-bome
documented connection options and config
3edec27
@adm-bome
small code style fix
6dd098f
@adm-bome
update php-amqplib
92a1ef4
@adm-bome
update php-amqplib
222ddb4 
 - reverted to minimal requirement update
@adm-bome
update php-amqplib
502bb75 
 - reverted to minimal requirement update
@adm-bome
added:
61f9142 
  - factory for creating a config object.

fixed:
  - tests

removed:
  - unnecessary methods
@adm-bome
cleanup
859feda
@adm-bome adm-bome requested review from M-Porter, vyuldashev and khepin and removed request for vyuldashev March 11, 2023 21:22
@adm-bome adm-bome changed the title Fix/refactor creation of connections and queues Fix/refactor creation of RabbitMQ Connection and QueueAPI Mar 13, 2023
adm-bome added 2 commits March 15, 2023 14:59
@adm-bome
cleanup:
7d1ef06 
  - Connection disconnected by default (Lazy)
  - refactored publish methods
@adm-bome
test lazy:
e25d51b 
  - Connection disconnected by default (Lazy)
khepin
khepin reviewed Mar 15, 2023
View reviewed changes
Copy link
Collaborator

@khepin khepin left a comment

Choose a reason for hiding this comment

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

I think the main structure is good.

The only thing I think should be updated is the use of final and private.

As it stands today this package uses the private keyword only once and never ever uses final. If memory serves well, a couple years ago, the opposite was true. I think it better to keep the same approach here.

src/Queue/Connection/ConfigFactory.php Outdated Show resolved Hide resolved
@adm-bome
updated:
e53fea0 
 - removed `final`
 - changed `private` to `protected`
@adm-bome adm-bome requested a review from khepin March 16, 2023 11:06
adm-bome added 2 commits March 16, 2023 13:20
@adm-bome
fixed:
453c890 
 - style testing
 - style fixing
@adm-bome
updated ReadMe
5acb1cd
@adm-bome adm-bome mentioned this pull request Mar 16, 2023
Merged
mattgrande
mattgrande approved these changes Mar 17, 2023
View reviewed changes
Copy link
Collaborator

@mattgrande mattgrande left a comment

Choose a reason for hiding this comment

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

Nice work on those factories; this is a huge improvement.

I've added a couple minor grammatical changes to the README, but other than that, this looks great.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Contracts/RabbitMQQueueContract.php Show resolved Hide resolved
adm-bome and others added 4 commits March 17, 2023 17:18
@adm-bome
refactor getExchangeType()
08fae3a 
 - Prior to PHP 8.0 constant() returned null and an E_WARNING, so default could be set easily. Sinds PHP 8.0 an ERROR exception is thrown.
 - added a defined() check.
@adm-bome
altered tests
9a900f7 
 - added null values in testing config
 - testing defaults
 - testing logic
@adm-bome
some extra cleanup
bf18827
@adm-bome @mattgrande
Update README.md
fa26c9c 
Co-authored-by: Matt Grande <[email protected]>
@adm-bome @mattgrande
Update README.md
a35587d 
Co-authored-by: Matt Grande <[email protected]>
@adm-bome adm-bome requested a review from mattgrande March 17, 2023 16:53
M-Porter
M-Porter approved these changes Mar 17, 2023
View reviewed changes
Copy link
Collaborator

@M-Porter M-Porter left a comment

Choose a reason for hiding this comment

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

This is great to see! I started working on a branch with factories but this is much better than any of the code I had.

Thank you for working on this!

adm-bome
adm-bome commented Mar 17, 2023
View reviewed changes
README.md Outdated Show resolved Hide resolved
@adm-bome
Copy link
Collaborator Author

@khepin @mattgrande @M-Porter

Could you guys, please have a new look at this PR.
I added some more minor changes / fixes.

If all is ok and tested, this can be merged. [version: v13.3.0]
@M-Porter can you make the neccesarry updates in the CHANGELOG when released?

adm-bome added a commit that referenced this pull request Mar 28, 2023
@adm-bome
Some rework based on PR #528
8da5b9d 
 - issue #460
 - rework based on [comment](#531 (comment))
@adm-bome adm-bome requested a review from M-Porter March 28, 2023 16:01
@khepin
Copy link
Collaborator

khepin commented Apr 10, 2023

Thanks @adm-bome this is great

@adm-bome adm-bome merged commit 9c0d172 into master Apr 11, 2023
@adm-bome adm-bome deleted the fix/refactor-creation-of-connections-and-queues branch April 11, 2023 08:24
@adm-bome adm-bome mentioned this pull request Apr 11, 2023
Closed
@M-Porter M-Porter mentioned this pull request Jun 15, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@khepin khepin khepin approved these changes

@mattgrande mattgrande Awaiting requested review from mattgrande

@M-Porter M-Porter Awaiting requested review from M-Porter

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adm-bome @khepin @mattgrande @M-Porter