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

NTR - Various updates #104

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

NTR - Various updates #104

wants to merge 3 commits into from

Conversation

cyl3x
Copy link
Contributor

@cyl3x cyl3x commented Aug 29, 2024 •
edited
Loading

  • Made use of composite github actions
  • Added eslint annotations to code review (github action)
  • Replaced ecs with php-cs-fixer

@cyl3x cyl3x added the enhancement New feature or request label Aug 29, 2024
@cyl3x cyl3x changed the title Ntr/various updates NTR - Various updates Aug 29, 2024
@cyl3x cyl3x force-pushed the ntr/various-updates branch 2 times, most recently from 43875f4 to 5b78392 Compare August 29, 2024 19:41
@cyl3x cyl3x requested a review from lernhart August 29, 2024 19:42
@cyl3x cyl3x self-assigned this Aug 29, 2024
@cyl3x cyl3x force-pushed the ntr/various-updates branch 2 times, most recently from e189317 to d718868 Compare September 9, 2024 09:40
@cyl3x cyl3x force-pushed the ntr/various-updates branch 2 times, most recently from 5716c9e to a5b4794 Compare September 25, 2024 15:39
@cyl3x cyl3x force-pushed the ntr/various-updates branch 2 times, most recently from d581054 to 098fa8b Compare October 7, 2024 11:53
@cyl3x cyl3x force-pushed the ntr/various-updates branch from 098fa8b to 346a0fa Compare November 14, 2024 15:54
@lernhart
Copy link
Member

lernhart commented Nov 19, 2024 •
edited
Loading

Can this be closed now ? All changes were implemented in other PRs, correct ? @cyl3x

I guess, just this commit is missing in trunk ?

@cyl3x cyl3x force-pushed the ntr/various-updates branch from 346a0fa to c7c24b1 Compare November 19, 2024 12:23
@cyl3x
Copy link
Contributor Author

cyl3x commented Nov 19, 2024

@lernhart exactly. I can't remember the reason for the RespectfulUuidGenerator and it's still subject to test, but doctrines UuidGenerator should fulfil our purposes

@lernhart
Copy link
Member

It's purpose is to not overwrite entities with given IDs.
Doctrine stupidly wimply overrides this on insert.

@cyl3x cyl3x force-pushed the ntr/various-updates branch 5 times, most recently from eaa762c to 15da9cf Compare November 20, 2024 14:19
@cyl3x cyl3x force-pushed the ntr/various-updates branch from 15da9cf to 94ac843 Compare November 20, 2024 14:20
@cyl3x
Copy link
Contributor Author

cyl3x commented Nov 20, 2024

@lernhart I don't think this problem is (still) valid. All objects are known to the object manager - even the deserialized ones trough $em-getReference(). I added an integration test for that, maybe have a look and try to reproduce the issue to solve 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lernhart lernhart Awaiting requested review from lernhart

At least 1 approving review is required to merge this pull request.

Assignees

@cyl3x cyl3x

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@cyl3x @lernhart