-
-
Notifications
You must be signed in to change notification settings - Fork 3k
Removing deprecated parts from the PageBundle #14244
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
Removing deprecated parts from the PageBundle #14244
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 6.x #14244 +/- ##
============================================
+ Coverage 63.64% 63.67% +0.03%
+ Complexity 34648 34627 -21
============================================
Files 2276 2276
Lines 103648 103580 -68
============================================
- Hits 65965 65954 -11
+ Misses 37683 37626 -57
|
@JonasLudwig1998 can you please rebase/merge and resolve the conflits? M6-alpha will be released in January so we should focus on refactoring PRs like this one. |
5db1e30
to
153834c
Compare
@escopecz Done |
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.
Just one small thing and then we can merge. I confirmed that none of the deleted classes/methods is used in the codebase.
UPGRADE-6.0.md
Outdated
- The `lightsaml/sp-bundle` package was replaced with a maintained fork `lightsaml2/sp-bundle` | ||
- `Mautic\PageBundle\Form\Type\PagePublishDatesType` was removed. | ||
- `getSessionName` was removed from `Mautic\PageBundle\Helper\TrackingHelper` No session for anonymous users. Use `getCacheKey`. | ||
- `getSession` was removed from `Mautic\PageBundle\Helper\TrackingHelper` No session for anonymous users. Use `getCacheItem`. | ||
- `updateSession` was removed from `Mautic\PageBundle\Helper\TrackingHelper` No session for anonymous users. Use `updateCacheItem`. | ||
- `getNewVsReturningPieChartData` was removed from `Mautic\PageBundle\Model\PageModel`. Use `getUniqueVsReturningPieChartData()` instead. |
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.
Please move these into the "BC breaks in the code" section
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.
Thank you! 👍
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.
Wait, is the change in .ddev/commands/host/phpmyadmin
intentioinal? Perhaps we could add it to .gitignore
instead? It's bothering me too. It's being generated by tests or something.
I like the idea. Should I change the .gitignore directly within this PR? |
Yep, let's do it here. I'll merge it right after. |
Not sure how to get rid of the changes in the .ddev/commands/host/phpmyadmin file. Any idea? |
4ca10ea
to
ba5bdc3
Compare
I chatGPTed it: When you add a file to Here's how to resolve the issue:
After doing this, the file should no longer show up in your PR because it's no longer tracked by Git, and your Note: Make sure any necessary backup of the file is done if it’s crucial, as removing it from the Git index will mean it won't be included in future commits, even if altered. The file is already there but with different permissions. I'm wondering whether we shouldn't just let the permissions be changed. |
Would be fine with me. Is it even necessary to put the file in .gitignore then. I'm currently struggling with the problem that the file is now deleted. I'll make sure that everything works properly tomorrow |
@escopecz Now I changed it back as it was. Is it fine this way, or should I change the permissions for the phpmyadmin file as they were before? |
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 think the permission change is fine. It's in ddev which is not meant for production so it does not matter as much as if it was meant for production. The previous comment on that files is 4e211eb which does not suggest any need for stricter permission. It shows up as a change all the time when developing so it's annoying. I think it's good to go 👍
This PR removes some of the deprecated parts in the PageBundle. It's based on the #14219 and should be merged after this one is merged