[meta] Add Change Record to @deprecated for common.inc

Hi @kwhite, I reviewed patch file provided in #4. Many of the @deprecated has been linked to appropriate change record. But there are still some @deprecated which need to be linked to @see. Lines with @deprecated which doesn't have @see are 181, 759, 864, 876, 898, 1014, 1032.

Thanks for the patch file.

1. +++ b/core/includes/common.inc
   @@ -129,6 +129,8 @@
   + * @see https://www.drupal.org/node/2571563
     */
    const LOCALE_PLURAL_DELIMITER = PluralTranslatableMarkup::DELIMITER;
   

   Correct.

2. +++ b/core/includes/common.inc
   @@ -154,6 +156,8 @@
   + * @see https://www.drupal.org/node/2448603
     */
    function drupal_get_destination() {
   

   Correct.

3. +++ b/core/includes/common.inc
   @@ -220,6 +224,8 @@ function valid_email_address($mail) {
   + * @see https://www.drupal.org/node/2560027
     */
    function check_url($uri) {
   

   Correct.

4. +++ b/core/includes/common.inc
   @@ -318,6 +324,8 @@ function format_size($size, $langcode = NULL) {
   + * @see https://www.drupal.org/node/1876852
     */
    function format_date($timestamp, $type = 'medium', $format = '', $timezone = NULL, $langcode = NULL) {
   

   Correct.

5. +++ b/core/includes/common.inc
   @@ -419,6 +427,8 @@ function base_path() {
   + * @see https://www.drupal.org/node/2317841
     */
    function drupal_clear_css_cache() {
   

   Correct.

6. +++ b/core/includes/common.inc
   @@ -736,6 +746,8 @@ function drupal_attach_tabledrag(&$element, array $options) {
   + * @see https://www.drupal.org/node/2317841
     */
    function drupal_clear_js_cache() {
   

   Correct.

That still leaves:

• valid_email_address()
• drupal_pre_render_link()
• drupal_render_root()
• drupal_render()
• drupal_render_children()
• element_info()
• element_info_property()

I shall have a wee look at this during the DrupalCon Vienna2017 code sprint. I'll be looking at valid_email_address() first.

I couldn't find a change record for this one, so have drafted a new one: https://www.drupal.org/node/2912661. Someone who knows about things will need to fix this, as I'm not totally confident I know what I am doing :-)

I will work on the drupal_pre_render_link during drupalcon vienna

Hi, I'm in Vienna I'll try "Retrieves the default properties for the defined element type." @line 1009

Hi, I am in Vienna too and I'll work on the method "element_info_property"

I will work on drupal_render_root()

I've just put the patch for the "element_info" method instead of element_info_property as discussed with @janec and @dinarcon.

So, element_info_property is still to do

drupal_render() - I am working on this. Vienna2017

I've created a patch to valid_email_address() that adds in the missing @see reference to the newly created Change Record for this. (See #9 for background.)

The change for the function drupal_render_root() happened in #2346937-38: Implement a Renderer service; reduces drupal_render / _theme service container calls and no change record was created. I draft a change record for this.

The function pre_render_link changed in #2305839: Convert hook_element_info() to annotated classes. The change record attached to it doesn't contain the information about pre_render_link. I will create a new one for it.

I couldn't find a change record for drupal_render_root(), so I have drafted a new one: https://www.drupal.org/node/2912696

Hi, I am at DrupalCon sprints.

I reviewed the last patch, and it does not include the changes made in the patch number 14.

Sorry about that! Here's the new patch including all of the previous ones.

I am going to look at drupal_render_children() next. That's the last one to be done here.

These are the changes added records for the pre_render_link

patch for element_info_property

drupal_render_root() change record updated to include drupal_root(). After discussion, the change is similar and happens in the same commit (da8ea3bfaa86cae97d5765968f696875c259f137).

https://www.drupal.org/node/2912696

Patch for drupal_render().

I have created the patch for drupal_render_root()

There was a mistake on the change record: https://www.drupal.org/node/2912696, I have just fixed it.

Generally, you want to include all changes in one patch, so that one person can submit it, and another person can review it. Set the issue status to 'Needs Review' and it will run the tests against the patch you've submitted.

There was a mistake on the change record: https://www.drupal.org/node/2235461
I have fixed it.

Hi @Mile23,

Totally agree. Sorry for the noise here. We are creating individual patches for each function so each sprinter gets acquainted with the process of working on issues and creating patches. We are also peer reviewing each other's work.

I have created the patch for drupal_render_children() and the change record is https://www.drupal.org/node/2912757.

At DrupalCon Vienna 2017, I mentored a group of sprinters who worked collaboratively on this issue:

@mairi, @billywardrop, @sthomps5, @Janec, @JO0st, @davemckain, and @davemckain investigated if changed records already existed and created/updated ones as appropriate.

@anyaabchiche did patch reviews and updated changes records.

Updating issue summary.

Amazing work on this, everyone! The research in the summary is very helpful and this issue is a big step for our update documentation and the smoother upgrade path.

In order to add these changes, let's create child issues for each of the change records in the summary here, and each child issue can also check for other places in core that also need to mention each change record. This is important to make sure we have complete scope for each change record (so someone doesn't have to repeat the research again for things related to these change records in different files). (@tedbow and I will help with this.)

Meanwhile, adding issue credit here for everyone who helped work on this at Vienna. We'll also make sure everyone is credited on the child issues.

@xjm thanks for the explanation here!

I created the new issues. There were 10. I made did make them childs of #2873705: Add change records to all @deprecated code directly though. I hope that was ok.

I added a reference this issue in all of the new issues so you can see them in the sidebar here under "Referenced by"

In each new issue in the summary you will find a reference to related change record.

Remember #2873705: Add change records to all @deprecated code gives very detailed instructions on how to handle these issues. So it is probably a good idea to read that over if you have not.

Thanks to everyone for identifying these changes @deprecated tags and as needing the change record links!

I added all the contributors from this issue to #2920018: Add Change record to @deprecated for element_info service for issue credit. :)

Let's keep this one open for now as a plan issue, so that contributors can follow the progress on the child issues.

I am very confused. :-(

According to the table in the issue summary, the deprecated const LOCALE_PLURAL_DELIMITER is covered by the change record TranslationManager->formatPlural() returns a PluralTranslatableMarkup. Two problems:

1. That change record does not mention LOCALE_PLURAL_DELIMITER.
2. That change record is wrong in several ways.

There is so much wrong here that I am afraid, in my confusion, to try and fix it.

The change records says,

In #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list we made TranslationManager->formatPlural() return a TranslatableMarkup object. To be consistent we now return a PluralTranslatableMarkup from TranslationManager->formatPlural().

What am I missing? We are being consistent by returning two different classes from the same method? Or are we changing from TranslatableMarkup to PluralTranslatableMarkup to be consistent with something else? If so, consistent with what?

Furthermore, the changes to TranslationManager.php for that issue do not involve the formatPlural() method, and those changes do not include the string (case insensitive search) "formatPlural".

Maybe what the change record means is that, after #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list, formatPlural() returned PluralTranslatableString, and then #2576533: Rename SafeStringInterface to MarkupInterface and move related classes changed the name of that class to PluralTranslatableMarkup (although the issue summary does not mention that). The change record does list both of those issues, but they are not mentioned in its description.

Finally, #2570107: Make format_plural() return a PluralTranslatableString object to remove reliance on a static, unpredictable safe list is the issue in which LOCALE_PLURAL_DELIMITER was marked as deprecated, even though the change record does not mention this.

I am feeling less confused than when I started writing this comment. I am still hesitant to make changes, but I think this is what is needed:

1. Rewrite the change record as described above ("what the change record means is ...").
2. Add a paragraph to the change record about deprecating LOCALE_PLURAL_DELIMITER.
3. Update the issue summary of #2576533 to include PluralTranslatableString -> PluralTranslatableMarkup.

The update of #2576533 should review all the classes involved in the renaming. If one was left out, then I am suspicious of the entire list.

Filed #3029336: Properly deprecate check_url()

Checked at least common.inc and after 9.0 all deprecated been removed