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

@type property on mercure's 'delete' message #2688

Merged
merged 2 commits into from Jan 4, 2023
Merged

@type property on mercure's 'delete' message #2688

merged 2 commits into from Jan 4, 2023

Conversation

toriqo
Copy link
Contributor

@toriqo toriqo commented Apr 4, 2019 •
edited

return the type so a client subscribed to more than one topic will know which resource type was deleted without having to parse the @id

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

@toriqo toriqo changed the title @type @type property on mercure's 'delete' message Apr 4, 2019
@toriqo
Copy link
Contributor Author

toriqo commented Apr 4, 2019

ping @dunglas @soyuka @antograssiot

soyuka
soyuka approved these changes Apr 4, 2019
View reviewed changes
Copy link
Member

@soyuka soyuka left a comment

Choose a reason for hiding this comment

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

I'm 👍 for the change but let's see what @dunglas has to say about this before merging.

dunglas
dunglas requested changes Apr 7, 2019
View reviewed changes
Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

Nice addition. Just a minor change to do. Thanks for this feature!

src/Bridge/Doctrine/EventListener/PublishMercureUpdatesListener.php Outdated Show resolved Hide resolved
@soyuka soyuka requested review from dunglas and antograssiot and removed request for dunglas April 8, 2019 14:50
@soyuka
Copy link
Member

soyuka commented Sep 30, 2019

needs to target master

@norkunas
Copy link
Contributor

This could be useful.. @toriqo are you up to finish this?

@bpolaszek
Copy link
Contributor

Just saying: the only way, client side, to know that a Mercure update is a removal instead of an update, is that you only have the @id property in the payload, right?

If so, why not considering this as a BC break?

(I 100% agree with adding the @type, that's not my point)

@divine
Copy link
Contributor

divine commented Nov 13, 2021

Hello,

I do really hope this will be a priority before the next major release. It's a needed feature that has been delayed for a few years.

@dunglas @soyuka does it needs only rebasing or something is left? Should I try to take care of it?

Thanks!

@dunglas
Copy link
Member

dunglas commented Nov 13, 2021

This just needs a rebase, and a decision about @id. I also fear that it's a BC break. I would suggest not setting it, or at least using a configuration option such as include_id under the mercure key.

@bpolaszek
Copy link
Contributor

This just needs a rebase, and a decision about @id. I also fear that it's a BC break. I would suggest not setting it, or at least using a configuration option such as include_id under the mercure key.

You mean @type and include_type, right?

@dunglas
Copy link
Member

dunglas commented Nov 14, 2021

Indeed.

@stale
Copy link

stale bot commented Nov 4, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Nov 4, 2022
@stale
Copy link

stale bot commented Jan 3, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jan 3, 2023
@bpolaszek
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Let's add a coin in the juke box - wasn't it just a minor change?

@stale stale bot removed the stale label Jan 4, 2023
@alanpoulain
feat: return the type in delete Mercure update
5690089 
A client subscribed to more than one topics will know which resource type was deleted without having to parse the @id.
@alanpoulain alanpoulain changed the base branch from 2.4 to main January 4, 2023 10:26
@alanpoulain
Copy link
Member

I've picked this one. WDYT @bpolaszek?

@bpolaszek
Copy link
Contributor

I've picked this one. WDYT @bpolaszek?

Sounds 👌

@alanpoulain
Copy link
Member

Let's merge it then. It will be available in 3.1. Thanks everyone.

@alanpoulain alanpoulain merged commit e5f1be0 into api-platform:main Jan 4, 2023
BacLuc added a commit to BacLuc/ecamp3 that referenced this pull request May 6, 2023
@BacLuc
api: set mercure.include_type to true
997d8bb 
Fixes:
585x: Since api-platform/core 3.1: Having mercure.include_type (always include @type in Mercure updates, even delete ones) set to false in the configuration is deprecated. It will be true by default in API Platform 4.0.
    4x in CreateColumnLayoutTest::testCreateRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\ColumnLayout
    4x in UpdateColumnLayoutTest::testPatchRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\ColumnLayout
    4x in CreateMaterialNodeTest::testCreateRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\MaterialNode
    4x in UpdateMaterialNodeTest::testPatchRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\MaterialNode
    4x in CreateMultiSelectTest::testCreateRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\MultiSelect

See api-platform/core#2688
@BacLuc BacLuc mentioned this pull request May 6, 2023
Merged
usu pushed a commit to ecamp/ecamp3 that referenced this pull request May 12, 2023
@BacLuc @usu
api: set mercure.include_type to true
69a04f4 
Fixes:
585x: Since api-platform/core 3.1: Having mercure.include_type (always include @type in Mercure updates, even delete ones) set to false in the configuration is deprecated. It will be true by default in API Platform 4.0.
    4x in CreateColumnLayoutTest::testCreateRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\ColumnLayout
    4x in UpdateColumnLayoutTest::testPatchRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\ColumnLayout
    4x in CreateMaterialNodeTest::testCreateRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\MaterialNode
    4x in UpdateMaterialNodeTest::testPatchRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\MaterialNode
    4x in CreateMultiSelectTest::testCreateRejectsParentsWhichDontSupportChildren from App\Tests\Api\ContentNodes\MultiSelect

See api-platform/core#2688
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@soyuka soyuka soyuka approved these changes

@antograssiot antograssiot antograssiot approved these changes

@dunglas dunglas Awaiting requested review from dunglas

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

Successfully merging this pull request may close these issues.

None yet
8 participants
@toriqo @soyuka @norkunas @bpolaszek @divine @dunglas @alanpoulain @antograssiot