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
renamed GalleryHasMedia to GalleryItem #948
Conversation
Can we merge this in 3.1 ? @soullivaneuh |
ping @greg0ire can you please review? |
'title' => $galleryItem->getMedia()->getName(), | ||
'caption' => $galleryItem->getMedia()->getDescription(), | ||
'type' => $type, | ||
'media' => $galleryItem->getMedia(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't we stop aligning things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i rebased, but this is old and should then be fixxed by Style CI IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok fine :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
👍 for such a change, this will finally clarify what my |
|
||
## GalleryHasMedia | ||
|
||
The `GalleryHasMedia` has been renamed to `GalleryItem`, please adjust your code and be sure to move your assets to the correct database table. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
gqq
on this line with vim
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
?? what do you mean? 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That it's too long. If you open the file with vim, and type 10Ggqq
, it will be fixed :)
Huge refactoring indeed! Congrats! |
Yes after I push 3.0. But it has to be BC. |
then lets merge it in 4.0, this is a massive change, all classes needs to be handeld... And i thought, because |
You rename the class |
Really? IMO we need to deprecate all (Api-)Controller classes too and maybe something else. and i don't know what will happen if you upgrade and upgrade your schema, the new table name shouldn't match the new generated one, shouldn't it? Wee need to keep the service name And the And the |
According to the new Sonata version management and next major release plan, this project has been refactored regarding branching and versioning. If you see this message, your PR concerns a patch or a minor release and is not targeting the right branch. So I'm closing this one, but don't see it as a refusal. If you think your work is still relevant and want to continue, feel free to reopen it on the right branch (e.g. the default one). Regards. |
This is for the next major |
Oh wait Travis has the same problems in fact : https://travis-ci.org/sonata-project/SonataMediaBundle/jobs/135807506#L623 |
So it must simply be that SonataCI is more strict |
Could you please rebase your PR and fix merge conflicts? |
rebased |
Could you please rebase your PR and fix merge conflicts? |
rebased again! Can we please merge this? |
Why is it marked as "in progress"? |
oops, sorry |
## Renamed GalleryHasMedia to GalleryItem | ||
|
||
All Actions, Controllers, Interfaces and anything related to this is renamed accordingly. | ||
>>>>>>> renamed GalleryHasMedia to GalleryItem |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
Thanks for the huge work @OskarStark ! |
Thank you for your feedback @greg0ire 👍 |
@OskarStark Is there a reason why we kept |
not really, did i forget it?
|
The file still exists, I would say yes |
I created a PR here: #1094 |
Fixes #828
Changelog
Subject
This change is to huge to keep it BC, so i will schedule this for the next major release.
@sonata-project/contributors please review this change, thank you!