- Issue created by @vishalkhode
- 🇮🇳India vishalkhode
vishalkhode → changed the visibility of the branch 3471672-the-module-fails to hidden.
- 🇮🇳India vishalkhode
vishalkhode → changed the visibility of the branch 3471672-the-module-fails to active.
- Status changed to Needs review
4 months ago 8:59pm 2 September 2024 - 🇮🇳India vishalkhode
There are couple of failures in Next Major CI, which are un-related to this issue, hence, I've not fixed this as part of this ticket. But If needed/requested, I'll fix those here. Hence, moving it to Needs Review.
- Issue was unassigned.
- Status changed to Closed: won't fix
4 months ago 5:17am 3 September 2024 - 🇮🇩Indonesia gausarts
Thank you.
Deprecation jobs are normally taken care of by Drupal Rector.
Manual works had been proven to fail and cause fatal errors somewhere as seen at Slick project, etc.. Not always, but a few times. Even when they were harmless at first glance. We should avoid wasting energy for manual works whenever bots can do better and safer jobs, specific for depreciation issues.
With due respects, I didn't want to repeat terrible experiences. Worst when the patchers didn't care enough to reply to my questions on the affected subject matters. I am a very patient man, I asked them six times very patiently and respectfully, and each question had never been replied properly, and when later I applied their patches, boom, I got errors somewhere they didn't even bother to answer in the first place. He repeated the same arrogant patches without explanation at this module, and I could no longer be patient. People who never read the entire story might think I was impatient, but rest assured I am a very patient man until the seventh annoying repetition. Be warned, I might reduce it to the third like other normal people once in a while, though.
You are good at communicating the problems and potential regressions, thank you! The previous patchers are horrible at communications. They should learn from you how to communicate an issue.
Please focus your efforts on helping other areas instead. Yours is useful for those in urgent need whenever bots come too late to party.
Appreciated, nevertheless.
- 🇮🇩Indonesia gausarts
I just have time to look back, so for documentation purposes:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/core.services.yml?ref_type=heads#L1691 library.discovery.collector: arguments: ['@cache.discovery', '@lock', '@library.discovery.parser', '@theme.manager'] class: Drupal\Core\Asset\LibraryDiscoveryCollector deprecated: The "%service_id%" service is deprecated in drupal:11.1.0 and is removed from drupal:12.0.0. Use LibraryDiscovery instead. See https://www.drupal.org/node/3462970
Meaning library.discovery.collector is deprecated for library.discovery. Not the opposite.
You appeared to misunderstand the CR, and your patch did the opposite, and so it will break D12.
As I said manual deprecation works are prone to fatal errors, some due to misunderstanding, the rest are carelessness. Hopefully you don't post this type of issues anywhere else, or you'll break them. If you did, be sure to close them out.
But don't take it so hard, just so we are looking at things right. We all made mistakes. What matters, we correct them whenever we can.
- 🇮🇳India vishalkhode
Hi @gausarts
Thanks for your suggestions, duly noted. I realize that I mistakenly updated the service fromlibrary.discovery
tolibrary.discovery.collector
, which is now deprecated. I believe this would only break in Drupal 12 (which hasn’t been released yet), correct? That’s why we have the review system on drupal.org—to ensure that reviewers can catch these mistakes, suggest changes, and help get them fixed before merging. I agree that mistakes happen, but that’s where the review process comes in to catch and correct them.The bigger issue here, in my opinion, is not that developers make incorrect changes in their MRs—nobody sets out to do that. The main problem is the absence of CI for this module. If we had CI, it would automatically test the module across all supported Drupal Core versions, flagging any test failures, deprecations, or code standards issues. Without this in place, it’s easy to merge changes that haven’t been fully tested across the different core versions.
Thanks. - 🇮🇩Indonesia gausarts
You are correct. Do not hesitate to post any more improvements you have in mind in another thread.
That would be very much appreciated!