Hey!

I think @WaylandAce has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

Hi everyone, what is the state of this PR?
Should it target 5.4 instead of 6.1? To me it looks like a bug fix and not a new feature. The current behaviour is broken.

I can confirm the provided patch works, a non-existent queue is created when calling ->setup() if needed.

Friendly ping @fabpot @jderusse @Nyholm 🙏

Thanks!

This was part of the original PR by @jderusse. It is not a bug it is a new feature.

I dont remember why this was added. Jeremy, do you know?

This is for 5.4. It just misses a test case (if possible)

I will create a new PR with @nuvolapl's commit + a new test case.

Sorry Tobias, I missed your comment. If creating the queue on the fly was not expected to work, then it is indeed a new feature.

Then what's the meaning of having an option auto_setup (which is true by default), if the auto-setup is not allowed?

This was part of the original PR by @jderusse. It is not a bug it is a new feature.

At this time, the DSN for the queue was sqs://default/queue_name OR sqs://default/owner_id/queue_name.
When owner_id was present, we presumed that it was somebodyelse's queue (like referencing the queue from another account). And, in that case, it was not possible de create the queue.

Since then, things changed:

• first, the owner_id could be us, and the presumption is wrong.
• the DSN could be now https://sqs.eu-west-1.amazonaws.com/owner_id/queue_name, and presumption is wrong again.

I believe this check should not be ignored because: If the code run with credentials belonging to account 1234, but queue is /9876/foo, the Symfony will create the queue foo on account 1234 which is wrong.

This line should be replaced by checking if the ownerId is identical to the currently connected account (see StsClient::getCallerIdentity).

Thank you for the clarification.

@Kocal do you understand and are happy moving forward in this way?

@chalasr With this new information (for both of us), do you agree on targeting 6.1?

Thanks everyone!

@Nyholm: yeah I'm fine with those explanations, I will try to send a PR ASAP.