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

GPS: revert the attributes and use the headers instead. #1355

Conversation

p-pichet
Copy link
Contributor

I found that i was go in wrong direction.
The GPS attributes are the Interop Message headers.

I revert to use the headers and remove the attributes

@p-pichet
Copy link
Contributor Author

@makasim can you review it please ?

@Steveb-p
Copy link
Contributor

@p-pichet the code you added, with explicit attributes, has just been released.

Technically, now you're breaking the changes you just introduced. Backwards compatibility breaks are a big no-no for libraries.

You have to explain it precisely why you want to go back, and what caused you to change your mind - links to resources, discussion that proves the issue, code samples, and so on.

@p-pichet
Copy link
Contributor Author

@Steveb-p, when I use it on my project, I don't have access to attributes in the consume message.

I check on the InteropQueue and found that I make a mistake of global compatibility.

Base on this comment :

The header contains fields used for message routing and identification;

The GPS attributes are simply headers.

By using a custom property attributes, we break the possibility to easily modify the transport by modify configuration.

What can I propose to avoid breaking change is this :
On the class GPSMessage :

  • keep getter and setter of attributes and mark them deprecated and remove them in the V1
  • on the getHeaders, merge the attributes with headers. If the both array have the same key, we keep the one in headers
    On the GPSProducer, we keep my refacto to send headers

What do you think ?

@p-pichet
Copy link
Contributor Author

@Steveb-p i push a commit to deprecate the attributes.

If you have time to take a look.

Copy link
Contributor

@Steveb-p Steveb-p left a comment

Choose a reason for hiding this comment

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

@Steveb-p i push a commit to deprecate the attributes.

If you have time to take a look.

The library is technically "not stable" being in 0.10 version series, so in theory it's fine to experiment.

A bit.

I appreciate your work done for the tests, but if you say that you need a particular feature and then it turns out it didn't work in your project, I will double check everything from now on.

I will look on it later on, but I will be busy in the next few days.

pkg/gps/GpsMessage.php Outdated Show resolved Hide resolved
pkg/gps/Tests/GpsMessageTest.php Show resolved Hide resolved
pkg/gps/GpsMessage.php Outdated Show resolved Hide resolved
@p-pichet p-pichet force-pushed the pubsub/use-headers-instead-of-attributes branch from aea4844 to c8c4993 Compare August 13, 2024 12:57
@p-pichet
Copy link
Contributor Author

@makasim I revert it.

@Steveb-p, i'm sorry. That was when I use it with sroze/messenger-enqueue-tranposrt that I saw I don't respect the 'standard' of interop enqueue.

@makasim makasim merged commit 0b3555a into php-enqueue:master Aug 13, 2024
68 checks passed
@Steveb-p
Copy link
Contributor

@p-pichet Don't worry :) It's just that when you're providing software for someone else, suddenly changes you can make in your own project that you were able to do are forbidden :)

If I can offer you a bit of advice - when working on a project and using open source libraries that you need to change to work, you can create your own forks and install them in place of the original.

See https://getcomposer.org/doc/05-repositories.md#repository .

For example, you can fork both enqueue-dev and sroze adapter, and do the changes that you need to. They can both be installed using VCS...

{
    "repositories": [
        {
            "type": "vcs",
            "url": "git@github.com:my-fork/enqueue-dev.git"
        }
    ]
}

...or even local paths:

{
    "repositories": [
        {
            "type": "path",
            "url": "../enqueue-dev-fork"
        }
    ]
}

And you can install branches pretending to be stable releases:

composer require "php-enqueue/enqueue-dev as 0.11"

(at least I think that's the syntax)

This is also one way of getting the features you need in your project, while waiting for your PRs to be approved.

@p-pichet
Copy link
Contributor Author

@Steveb-p thanks for the tips.

@makasim thanks for the merge. When do you think you will create the new release ?

@makasim
Copy link
Member

makasim commented Aug 14, 2024

https://github.com/php-enqueue/enqueue-dev/releases/tag/0.10.22

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.

3 participants