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

Decouple the calendar, FAQ and news bundles from the comments bundle #6650

Merged
merged 36 commits into from Jan 4, 2024
Merged

Decouple the calendar, FAQ and news bundles from the comments bundle #6650

merged 36 commits into from Jan 4, 2024

Conversation

zoglo
Copy link
Contributor

@zoglo zoglo commented Dec 21, 2023 •
edited

Feature

Description

  • This PR removes the hard dependency of the comment-bunndle within faq, news and calendar by using the PaletteManipulator
  • All language files have been separated as well

Following things haven't been removed

  • Comment implementation within the respecitve Modules
  • Comment implementation within Models
  • Comment implementation within Templates

Related

Disclaimer

The amount of files and lines changed is due to me actually moving the translations of multiple languages into the comments-bundle

@leofeyer leofeyer added this to the 5.3 milestone Dec 21, 2023
aschempp
aschempp requested changes Dec 21, 2023
View reviewed changes
Copy link
Member

@aschempp aschempp left a comment •
edited

Choose a reason for hiding this comment

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

Great improvement! There is one major problem though. Installing the comments bundle will now define fields on e.g. tl_calendar_events, even if the calendar bundle is not installed. We should either check for that in the DCA (which is kinda hacky) or dynamically add the fields using a loadDataContainer callback.

@zoglo
Copy link
Contributor Author

zoglo commented Dec 21, 2023

Great improvement! There is one major problem though. Installing the comments bundle will now define fields on e.g. tl_calendar_events, even if the calendar bundle is not installed. We should either check for that in the DCA (which is kinda hacky) or dynamically add the fields using a loadDataContainer callback.

I'll get onto it later this evening :)

@zoglo
Conditionally load fields via DataContainerListener
bf4f0f3 
- use loadDataContainer Hook
- universal size
  - notify is now varchar(32) for faq and news
  - sortOrder is now varchar(32) for faq
@zoglo
CS
a95005a
@zoglo
CS
cdc29f2
@zoglo
CS
41a2d1e 
- satisfy linter
@zoglo
Merge branch '5.x' into feature/comments-conditional-fields
0b7a6c9
@zoglo zoglo requested a review from aschempp December 22, 2023 01:27
aschempp
aschempp requested changes Dec 22, 2023
View reviewed changes
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

Very good! This shows how much duplicate code there was until now. Just a few minor things to fix for me 😎

comments-bundle/src/ContaoManager/Plugin.php Outdated Show resolved Hide resolved
comments-bundle/src/EventListener/DataContainer/CommentsConditionalFieldsListener.php Outdated Show resolved Hide resolved
comments-bundle/src/EventListener/DataContainer/CommentsConditionalFieldsListener.php Outdated Show resolved Hide resolved
comments-bundle/src/EventListener/DataContainer/CommentsConditionalFieldsListener.php Outdated Show resolved Hide resolved
comments-bundle/contao/dca/tl_module.php Outdated Show resolved Hide resolved
comments-bundle/src/DependencyInjection/ContaoCommentsExtension.php Outdated Show resolved Hide resolved
@zoglo
Merge branch '5.x' into feature/comments-conditional-fields
31e937a
@zoglo
Improve DataContainerListener and more
6f0f557 
- do not use legacy module names anymore
- unionize faq, comments and news withing palette manipulators
- CS
@zoglo
CS
fe5d32b 
- remove unused Use statement
@zoglo
Updates to DataContainerListener
32a76aa 
- register listener within services.yaml (and remove listener.yaml)
- get ``kernel.bundles`` via argument
- properly call methods (object and not class)
@zoglo zoglo requested a review from fritzmg December 25, 2023 12:32
@zoglo zoglo requested a review from aschempp December 27, 2023 17:52
aschempp
aschempp previously approved these changes Dec 28, 2023
View reviewed changes
Copy link
Member

@aschempp aschempp left a comment

Choose a reason for hiding this comment

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

🎉

Toflar
Toflar previously approved these changes Dec 28, 2023
View reviewed changes
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Wohoo, thanks so much for taking the time to finally separate this! Not a feature a user will ever see (other than not having useless fields in the table anymore 🥳 ) but a very valuable cleanup-contribution! ❤️

@Toflar
Copy link
Member

Toflar commented Dec 28, 2023

We'll probably need a test for the new CommentsConditionalFieldsListener though.

@zoglo zoglo dismissed stale reviews from Toflar and aschempp via 288da1d December 28, 2023 14:54
Toflar
Toflar previously approved these changes Dec 28, 2023
View reviewed changes
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Perfect!

@zoglo zoglo requested a review from Toflar December 28, 2023 15:31
Toflar
Toflar previously approved these changes Dec 29, 2023
View reviewed changes
@zoglo zoglo requested a review from leofeyer January 3, 2024 18:05
@leofeyer leofeyer linked an issue Jan 4, 2024 that may be closed by this pull request
Add conditional fields instead of removing them #3510
Closed
@leofeyer leofeyer changed the title Feature/comments conditional fields Decouple the calendar, FAQ and news bundles from the comments bundle Jan 4, 2024
leofeyer
leofeyer approved these changes Jan 4, 2024
View reviewed changes
Copy link
Member

@leofeyer leofeyer left a comment

Choose a reason for hiding this comment

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

Perfect. Than you very much @zoglo.

@leofeyer leofeyer merged commit ad07d3e into contao:5.x Jan 4, 2024
16 checks passed
@zoglo zoglo deleted the feature/comments-conditional-fields branch January 4, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leofeyer leofeyer leofeyer approved these changes

@Toflar Toflar Toflar left review comments

@aschempp aschempp aschempp left review comments

@fritzmg fritzmg fritzmg left review comments

Assignees

@zoglo zoglo

Labels
feature
Projects
None yet
Milestone
5.3
Development

Successfully merging this pull request may close these issues.

Add conditional fields instead of removing them
5 participants
@zoglo @Toflar @aschempp @leofeyer @fritzmg