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

Jump to bottom

[Workflow]Fix Marking when it must contains more than one tokens #53865

Merged
merged 1 commit into from Mar 11, 2024
Merged

[Workflow]Fix Marking when it must contains more than one tokens #53865

merged 1 commit into from Mar 11, 2024

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Feb 8, 2024 •
edited

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #53179
License MIT

@carsonbot carsonbot added this to the 7.1 milestone Feb 8, 2024
@lyrixx lyrixx mentioned this pull request Feb 8, 2024
Closed
nicolas-grekas
nicolas-grekas reviewed Feb 9, 2024
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

the word "partial" means nothing to the reader i think (it doesn't to me at least) :)

src/Symfony/Component/Workflow/CHANGELOG.md Outdated Show resolved Hide resolved
@carsonbot carsonbot changed the title [Worflow] Add partial support for marking with more than one token per place [Workflow] [Worflow] Add partial support for marking with more than one token per place Feb 12, 2024
@lyrixx lyrixx requested a review from chalasr as a code owner February 24, 2024 06:22
@lyrixx lyrixx changed the base branch from 7.1 to 5.4 February 24, 2024 06:22
@lyrixx lyrixx changed the title [Workflow] [Worflow] Add partial support for marking with more than one token per place [Worflow] Fix Marking when it must contains more than one tokens Feb 24, 2024
@lyrixx
Copy link
Member Author

lyrixx commented Feb 24, 2024

I updated the PR. Now it's a bugfix, and not a new feature

@lyrixx lyrixx removed the Feature label Feb 24, 2024
@carsonbot carsonbot changed the title [Worflow] Fix Marking when it must contains more than one tokens [Workflow] [Worflow] Fix Marking when it must contains more than one tokens Feb 24, 2024
@lyrixx lyrixx changed the title [Workflow] [Worflow] Fix Marking when it must contains more than one tokens [Workflow]Fix Marking when it must contains more than one tokens Feb 24, 2024
@lyrixx lyrixx force-pushed the workflow-multiple-token branch 3 times, most recently from b2f1dd0 to 175a20b Compare February 24, 2024 06:40
@lyrixx lyrixx modified the milestones: 7.1, 5.4 Mar 11, 2024
@lyrixx lyrixx merged commit 5ce412b into symfony:5.4 Mar 11, 2024
11 of 12 checks passed
@lyrixx lyrixx deleted the workflow-multiple-token branch March 11, 2024 15:51
chalasr
chalasr reviewed Mar 11, 2024
View reviewed changes
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Reading the code and the linked issue, I don't think this should have been merged as a bugfix.

src/Symfony/Component/Workflow/Marking.php
}

public function unmark(string $place)
public function unmark(string $place, int $nbToken = 1)
Copy link
Member

Choose a reason for hiding this comment

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

this change is a BC break for child classes overriding the method with extra parameter(s). It should rather be done the smooth way on 7.1

lyrixx added a commit that referenced this pull request Mar 12, 2024
@lyrixx
Revert "bug #53865 [Workflow]Fix Marking when it must contains more t…
10d7d43 
…han one tokens (lyrixx)"

This reverts commit 5ce412b, reversing
changes made to 7dae80d.
lyrixx added a commit that referenced this pull request Mar 12, 2024
@lyrixx
Merge branch '5.4' into 6.4
34f84cf 
* 5.4:
  Revert "bug #53865 [Workflow]Fix Marking when it must contains more than one tokens (lyrixx)"
lyrixx added a commit that referenced this pull request Mar 12, 2024
@lyrixx
Merge branch '6.4' into 7.0
40cd30b 
* 6.4:
  Revert "bug #53865 [Workflow]Fix Marking when it must contains more than one tokens (lyrixx)"
lyrixx added a commit that referenced this pull request Mar 12, 2024
@lyrixx
Merge branch '7.0' into 7.1
37d91c7 
* 7.0:
  Revert "bug #53865 [Workflow]Fix Marking when it must contains more than one tokens (lyrixx)"
@lyrixx
Copy link
Member Author

lyrixx commented Mar 12, 2024

Sorry for this mistake. the PR was open for a long period, so I thought it was okay.

I have reverted the merge, and I'll open a new PR against 7.1 ASAP

This was referenced Apr 2, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@chalasr chalasr chalasr left review comments

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Status: Needs Review Workflow
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

[Workflow] Verify race condition
5 participants
@lyrixx @nicolas-grekas @stof @chalasr @carsonbot