Skip to content

Conversation

JonasLudwig1998
Copy link
Contributor

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

Copy link

codecov bot commented Nov 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.67%. Comparing base (fd68014) to head (ff7768b).
Report is 42 commits behind head on 6.x.

Additional details and impacted files

Impacted file tree graph

@@             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     
Files with missing lines Coverage Δ
app/bundles/PageBundle/Helper/TrackingHelper.php 70.45% <ø> (+9.67%) ⬆️
app/bundles/PageBundle/Model/PageModel.php 55.44% <ø> (+1.57%) ⬆️

... and 23 files with indirect coverage changes

@escopecz
Copy link
Member

@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.

@escopecz escopecz added the has-conflicts Pull requests that cannot be merged until conflicts have been resolved label Dec 20, 2024
@JonasLudwig1998 JonasLudwig1998 force-pushed the fork/authentication-deprecations-assets branch from 5db1e30 to 153834c Compare January 2, 2025 09:09
@JonasLudwig1998
Copy link
Contributor Author

@escopecz Done

Copy link
Member

@escopecz escopecz left a 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
Comment on lines 103 to 108
- 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.
Copy link
Member

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

@escopecz escopecz added pending-feedback PR's and issues that are awaiting feedback from the author landing-pages Anything related to landing pages refactoring The change does not change behavior but improves the code and removed has-conflicts Pull requests that cannot be merged until conflicts have been resolved labels Jan 6, 2025
@escopecz escopecz added this to the 6.0 milestone Jan 6, 2025
Copy link
Member

@escopecz escopecz left a comment

Choose a reason for hiding this comment

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

Thank you! 👍

Copy link
Member

@escopecz escopecz left a 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.

@JonasLudwig1998
Copy link
Contributor Author

I like the idea. Should I change the .gitignore directly within this PR?

@escopecz
Copy link
Member

escopecz commented Jan 8, 2025

Yep, let's do it here. I'll merge it right after.

@JonasLudwig1998
Copy link
Contributor Author

Not sure how to get rid of the changes in the .ddev/commands/host/phpmyadmin file. Any idea?

@JonasLudwig1998 JonasLudwig1998 force-pushed the fork/authentication-deprecations-assets branch from 4ca10ea to ba5bdc3 Compare January 8, 2025 09:45
@escopecz
Copy link
Member

escopecz commented Jan 8, 2025

I chatGPTed it:


When you add a file to .gitignore, it will prevent future changes to that file from being tracked. However, if the file is already being tracked by Git (i.e., it's in the repository's history), .gitignore alone won't stop it from showing up in your PR if it's modified. That's likely why you're still seeing changes to the file permissions (100644 → 100755) in your pull request.

Here's how to resolve the issue:

  1. Remove the file from the Git index but keep it in your working directory:

    git rm --cached .ddev/commands/host/phpmyadmin

    This command removes the file from the Git index (i.e., it tells Git to stop tracking it) but leaves the file intact in your working directory.

  2. Commit the change:

    git commit -m "Stop tracking the phpmyadmin file"
  3. Push the changes to your branch:

    git push origin <your-branch-name>

After doing this, the file should no longer show up in your PR because it's no longer tracked by Git, and your .gitignore entry will prevent it from being tracked in the future.

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.

@JonasLudwig1998
Copy link
Contributor Author

JonasLudwig1998 commented Jan 8, 2025

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

@JonasLudwig1998
Copy link
Contributor Author

@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?

Copy link
Member

@escopecz escopecz left a 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 👍

@escopecz escopecz added code-review-passed PRs which have passed code review and removed pending-feedback PR's and issues that are awaiting feedback from the author labels Jan 9, 2025
@escopecz escopecz merged commit 163a3a6 into mautic:6.x Jan 9, 2025
17 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-review-passed PRs which have passed code review landing-pages Anything related to landing pages refactoring The change does not change behavior but improves the code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants