Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
unicode_requirements() is just a helper for system_requirements().
Proposed resolution
Move or copy the logic to system_requirements().
Deprecate unicode_requirements() for removal in 9.x, although in this case we might be able to get away with removing it in a minor since it should never, ever be called.
Comments
Comment #4
Alka Kumari CreditAttribution: Alka Kumari as a volunteer commentedComment #5
Alka Kumari CreditAttribution: Alka Kumari as a volunteer commentedI have brought the unicode_requirements() logic to system_requirements() in the patch.
Comment #6
catchWhile the chances of this being called are slim, we should do the following:
1. Copy the function contents over, as is done here
2. Leave the old function as is, but add deprecation notices per https://www.drupal.org/core/deprecation
Comment #7
Alka Kumari CreditAttribution: Alka Kumari as a volunteer commented@catch How to find out nid for "@see http://drupal.org/node/the-change-notice-nid" ?
Comment #8
Alka Kumari CreditAttribution: Alka Kumari as a volunteer commentedPatch including deprecation notice.
Comment #9
catchGoing to discuss the deprecation with @xjm a bit more, it's a shame we have to leave the copy - thanks for re-rolling with that in though.
More of an issue, we should inline the logic inside system_requirements() - the issue summary says this but the title isn't clear.
Comment #10
catchComment #11
xjmBTW, the patch in #8 is also not correct. It still removes the old function, and puts the deprecation notice on the new function. Neither of those things are things that we want. :)
We need to leave
unicode_requirements()
in place, follow the full deprecation policy for it (reference: https://www.drupal.org/core/deprecation) and then (unfortunately, as above) copy the code and just inline it where it's called insystem.install
.Thanks!
Comment #12
xjmOh, @Alka Kumari, for your question about how to get the node ID of the change record. Since we are adding the deprecation in this issue, we will need to create the change record here in this issue as well. Here's our handbook page on how to create change records for issues: https://www.drupal.org/contributor-tasks/draft-change-record
Comment #13
pk188 CreditAttribution: pk188 at OpenSense Labs commentedRe rolled #5 and fixed #6.
Comment #14
xjmThanks! A few more things.
It wasn't deprecated in 8.0.0; it's being deprecated for 8.4.0.
Also, minor nit: There should be a blank line between the @deprecated and @see.
Finally, this should include exactly the same text as the
@trigger_error()
, except with the documentation tags instead of words. See https://www.drupal.org/core/deprecation#how-function for an example.So, there's a missing space after the period. But let's rewrite this whole paragraph to say:
Note how the wording for the @trigger_error() is slightly different, since it's a user-facing sentence rather than a docblock. You can see the format at: https://www.drupal.org/core/deprecation#how-function
Also, note above that we still need a change record and that should be linked instead of this issue node. See https://www.drupal.org/contributor-tasks/draft-change-record for instructions on writing a change record.
Comment #15
xjmAlso, @pk188, please be sure to provide an interdiff when you make updates to a patch. Thanks!
Comment #16
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is an updated patch and interdiff.txt
Comment #17
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedThis is for the first time , I am creating the change record.
URL : https://www.drupal.org/node/2884698
Let me know, if I am missing anything.
Comment #18
Alka Kumari CreditAttribution: Alka Kumari as a volunteer commentedComment #19
xjmThanks @Dinesh18, that looks pretty good. The change record is a good start also. There is one tiny formatting error in the patch:
This line is missing its asterisk.
For reviewers, note that although the interdiff from #16 includes the patch file, the patch itself is correct.
I've added a bit more detail to the change record explaining why it's deprecated, and clarifying that the function itself should simply not be used (as
system_requirements()
also should not be called directly).Comment #20
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedThanks @xjm.
I will update the patch shortly.
Comment #21
Dinesh18 CreditAttribution: Dinesh18 as a volunteer commentedHere is the updated patch which fix the missing asterisk.
I have reviewed the change record, it's much better than what I have wrote initially. :)
Thanks @xjm.
Comment #22
xjmAlright great, I think this is ready! Thanks @Dinesh18 and @Alka Kumari.
Comment #24
catchCommitted/pushed to 8.4.x, thanks!
Comment #26
xjm@Calystod noticed that we linked the issue instead of the change record for this, so I've reverted the change to fix that since it was a mistake in the commit. Both @catch and I should have noticed this. :) We can just update the patch to link the CR.
Comment #27
clairedesbois@gmail.comNew patch with the good link to the changes record page.
Comment #28
clairedesbois@gmail.comComment #29
clairedesbois@gmail.comAnd the interdiff between patch 21 and 27.
Comment #30
xjmIt looks like the original patch linked the change record in the
@trigger_error()
but this issue in the docblock. The updated patch fixes that.Back to RTBC, thanks @Calystod! Good catch.
Comment #31
xjmComment #33
catchOh well spotted. I always check the link because linking back to the issue is a common mistake, but only checked one link and didn't notice they were different :P
Committed/pushed to 8.4.x, thanks!
Comment #35
scarletdrupal123 CreditAttribution: scarletdrupal123 commentedEven after setting unicode , i am not able to store french characters. Getting database exception every time.