There are some points of the codebase that mention Drupal 8, either in documentation or in code workaround. Remove the workarounds, update the info files to say "^9" instead of "^8 || ^9", and update everywhere to say "Drupal 9".

Issue fork metatag-3252359

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

DamienMcKenna created an issue. See original summary.

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue summary: View changes
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Issue tags: +Novice
murilohp’s picture

murilohp commented
Assigned: Unassigned » murilohp

murilohp opened merge request !36

murilohp’s picture

murilohp commented
Assigned: murilohp » Unassigned
Status: Active » Needs review

Hey, I tried to update all the places mentioning "Drupal 8", the code base is huge, so if I miss something, please let me know.

Thanks!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Needs work

That's a great step in the right direction, thank you!

One minor thing to change:

-This version should work with Drupal 8.8, 8.9, 9.0 and 9.1 releases, though it
+This version should work with Drupal 9.0, 9.1 and releases, though it

Please just change this to ".. work with all Drupal 9 releases".

Also, there's a workaround in the migration logic somewhere that checks whether the site is running on core 8.9, that bit needs to be removed too.

murilohp’s picture

murilohp commented
Assigned: Unassigned » murilohp

Thanks! I'll check this later!

murilohp’s picture

murilohp commented

@damienmckenna, I updated the code with the minor fix!

About the:

there's a workaround in the migration logic somewhere that checks whether the site is running on core 8.9, that bit needs to be removed too.

I haven't found anything about it, what I found was some codes like:

floatval(\Drupal::VERSION) <= 8.3

This type of the code is being repeated on the following files:

  • tests/src/Functional/MetatagFieldNodeTest.php ::84
  • tests/src/Functional/MetatagForumTest.php ::81
  • tests/src/Functional/MetatagXssTest.php ::155, 188, 209
  • tests/src/Kernel/Migrate/d7/MetatagEntitiesTest.php ::81
  • tests/src/Functional/MetatagNodeTranslationTest.php ::88
  • tests/src/Functional/MetatagStringTest.php ::164, 197, 269

Could you give more details about it? And regarding the tests, do you think we need to update them?

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Yes, any of the code that checks for (\Drupal::VERSION can be removed because it was all for Drupal 8.

murilohp’s picture

murilohp commented
Assigned: murilohp » Unassigned
Status: Needs work » Needs review

Got the idea, I updated the tests removing the unnecessary drupal 8 validation, I got a lot of warnings regarding deprecated code, I haven't changed this because I think it's not the scope of this issue, but in the future we can create a new ticket to replace:

$this->drupalPostForm()

To:
$this->submitForm()

The drupalPostForm will be removed in the future.

Moving to needs review, if you need anything please let me know!

Thanks!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented

Excellent, good work!

One minor thing - can you please remove the $this->t() bit around strings in the tests? Those are unnecessary. Thank you.

murilohp’s picture

murilohp commented

Hey, I removed the t calls from the parts that I changed, there's actually a lot of these calls inside the test classes, do you think it could be a good idea to create a new issue to work specifically with that?

Thanks!

damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Needs review » Reviewed & tested by the community

Yeah, we can do a separate issue for the additional work.

For now this is great work, thank you!

murilohp’s picture

murilohp commented

Great! I hope this can be merged in a while.

Thanks!

  • DamienMcKenna committed c3cc3d3 on 8.x-1.x authored by murilohp 
    Issue #3252359 by murilohp, DamienMcKenna: Remove workarounds for Drupal...
damienmckenna’s picture

damienmckenna
Location TN, USA
commented
Status: Reviewed & tested by the community » Fixed

Committed. Thank you.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.