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

BUGFIX: Cleanup media browser addFlashMessageTrait #3942

Merged

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Oct 31, 2022

resolves: #3898

another fix for the problem, by refactoring the AddFlashMessageTrait

thanks @crydotsnake for the initial effort with #3940 but it if accepted it would make your approach obsolete. (I dindt wanted to force push on your branch - thus a new pr ^^)

see #3940 (comment) for the explanation of this fix

Upgrade instructions

Review instructions

Create a tag with % sign and wait for the flashmessage:

image

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch
  • Reviewer - PR Title is brief but complete and starts with FEATURE|TASK|BUGFIX
  • Reviewer - The first section explains the change briefly for change-logs
  • Reviewer - Breaking Changes are marked with !!! and have upgrade-instructions

@mhsdesign mhsdesign changed the title Bugfix/3898 cleanup media browser add flash message trait Bugfix: 3898 cleanup media browser add flash message trait Nov 1, 2022
Copy link
Member

@crydotsnake crydotsnake left a comment

Choose a reason for hiding this comment

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

From the functionality point of view it looks good. I have only deposited a few notes regarding codestyle, and naming.

@crydotsnake crydotsnake changed the title Bugfix: 3898 cleanup media browser add flash message trait BUGFIX: 3898 cleanup media browser add flash message trait Nov 1, 2022
@mhsdesign mhsdesign force-pushed the bugfix/3898-cleanup-media-browser-AddFlashMessageTrait branch from bf26152 to e75e98e Compare November 1, 2022 15:00
@mhsdesign mhsdesign changed the title BUGFIX: 3898 cleanup media browser add flash message trait BUGFIX cleanup media browser add flash message trait Nov 1, 2022
@mhsdesign mhsdesign changed the title BUGFIX cleanup media browser add flash message trait BUGFIX Cleanup media browser add flash message trait Nov 1, 2022
@mhsdesign mhsdesign changed the title BUGFIX Cleanup media browser add flash message trait BUGFIX: Cleanup media browser add flash message trait Nov 1, 2022
@mhsdesign mhsdesign changed the title BUGFIX: Cleanup media browser add flash message trait BUGFIX: Cleanup media browser addFlashMessageTrait Nov 2, 2022
@kdambekalns
Copy link
Member

TBH: I don't see a difference in the code (except for naming). Is there any?

Copy link
Member

@kdambekalns kdambekalns left a comment

Choose a reason for hiding this comment

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

by reading

@crydotsnake
Copy link
Member

Tested, and works!

@crydotsnake crydotsnake merged commit b44ce25 into 7.3 Nov 12, 2022
@crydotsnake crydotsnake deleted the bugfix/3898-cleanup-media-browser-AddFlashMessageTrait branch November 12, 2022 09:53
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.

BUG: FlashMessages - current escaping of tags / collections leads to an exception
3 participants