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.

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

Feature/new66 #4932

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Feature/new66 #4932

wants to merge 12 commits into from

Conversation

alexwu-jcc
Copy link
Contributor

This is the PR that handles the task to archive all the news release links older than 5 years, by going to this route: /admin/jcc_news_archive, after having the custom module jcc_news_archive installed.

Copy link
Contributor

@melwong-jcc melwong-jcc left a comment

Choose a reason for hiding this comment

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

Rather than a separate module, can we combine everything into one module?

@Ravi-jud is working on the delete feature at web/modules/custom/jcc_newslinks/jcc_newslinks.module.

@alexwu-jcc
Copy link
Contributor Author

alexwu-jcc commented Jan 28, 2025 via email

@melwong-jcc
Copy link
Contributor

Though it's a different category for the same content type, it still can be combined into bundle of features for everything Newslinks related. I don't see a need for a separate module if it all pertains for the same content type unless you are saying that the cron job is the conflict.

Not a big issue either since the code is fine. Just I prefer to have related features bundled unless they must absolutely need to be installed separately. We need to be mindful on how much should be module vs our core. If you feel strongly to create a separate module, you should be okay to proceed.

@alexwu-jcc
Copy link
Contributor Author

Though it's a different category for the same content type, it still can be combined into bundle of features for everything Newslinks related. I don't see a need for a separate module if it all pertains for the same content type unless you are saying that the cron job is the conflict.

Not a big issue either since the code is fine. Just I prefer to have related features bundled unless they must absolutely need to be installed separately. We need to be mindful on how much should be module vs our core. If you feel strongly to create a separate module, you should be okay to proceed.

Thank you for revealing your thinkings. I agree that in most cases we should try to bundle related code together, however, this is a code and rare use case, and what I have put in the code is like create a backdoor and allow admin to use it when it is really needed. This logics, if bundled with others, then it will consume significant CPU power unnecessarily thus will drag down our sites' performance, because in that way the program logic will evaluate a condition, which essentially pulls the back end and tests for if there are any news release links older than five years, before proceeding to execute, no matter if that condition has been met or not. As you can tell, the backend pulling means at least a table scan plus network delay, and the enumerating of those records in memory also means some cost of CPU cycles. Yes, making our code maintainable is important, but even more important is the efficiency of our program. Hope you can approve this PR. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants