Problem/Motivation

TermInterface::getVocabularyId() is useless, we have TermInterface::bundle() inherited from \Drupal\Core\Entity\EntityInterface.

Proposed resolution

Deprecate the interface method.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#8 2850691-8.patch4.87 KBclaudiu.cristea
#6 2850691-4.patch4.86 KBclaudiu.cristea
#6 interdiff-4-to-6.txt1.33 KBclaudiu.cristea
#4 interdiff-2-to-4.txt776 bytesclaudiu.cristea
#4 2850691-4.patch4.86 KBclaudiu.cristea
#2 2850691-2.patch4.84 KBclaudiu.cristea

Comments

claudiu.cristea created an issue. See original summary.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Active » Needs review
StatusFileSize
new 2850691-2.patch4.84 KB

Patch

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented 
+++ b/core/modules/taxonomy/src/Entity/Term.php
@@ -229,7 +229,8 @@ public function setWeight($weight) {
+    @trigger_error('The getVocabularyId() method is deprecated since version 8.4.x and will be removed in 9.0.x. Use bundle() instead to get the vocabulary ID.', E_USER_DEPRECATED);

Let's use __METHOD__ instead and full version strings.

What about the following?

@trigger_error('The ' . __METHOD__ . ' method is deprecated since version 8.4.0 and will be removed in 9.0.0. Use ' . __CLASS__' . '::bundle() instead to get the vocabulary ID.', E_USER_DEPRECATED);

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new 2850691-4.patch4.86 KB
new interdiff-2-to-4.txt776 bytes

That's OK, too :)

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented 
+++ b/core/modules/taxonomy/src/TermInterface.php
@@ -87,6 +87,9 @@ public function setWeight($weight);
+   * @deprecated Scheduled for removal in Drupal 9.0.x. Use

Sorry, this should be 9.0.0 as well :)

Also looking at the most cases in core the "Use.." sentence is on the next line. Also probably instead of "Use TermInterface::bundle() instead." it is probably better to put "Use self::bundle() instead."

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
StatusFileSize
new interdiff-4-to-6.txt1.33 KB
new 2850691-4.patch4.86 KB

Sorry, this should be 9.0.0 as well :)

No, it should not -- that is incorrect. You remove the method either "in 9.0.x" or "before 9.0.0". You just cannot remove the method exactly in 9.0.0. Theoretically you could but that doesn't happen. The deprecated methods are removed during 9.0.x development interval. Let's go with "before" version.

Also looking at the most cases in core the "Use.." sentence is on the next line.

"Most cases" doesn't mean "correct" :) Quoting from https://www.drupal.org/docs/develop/coding-standards/api-documentation-a...

Lines containing comments (including docblocks) must wrap as close to 80 characters as possible without going over, with a few exceptions

Note the "(...) wrap as close to 80 characters as possible" part.

Also probably instead of "Use TermInterface::bundle() instead." it is probably better to put "Use self::bundle() instead."

Well, we are here on the interface. We should refer to methods belonging to interface. And ::bundle() belongs to this interface even is inherited. I would not change that.

Status: Needs review » Needs work

The last submitted patch, 6: 2850691-4.patch, failed testing.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented
Status: Needs work » Needs review
StatusFileSize
new 2850691-8.patch4.87 KB

Consider this patch for #6. That was wrong.

hchonov’s picture

hchonov
Primary language Bulgarian
Location Burgas
commented
Status: Needs review » Reviewed & tested by the community

Ok, I am fine with it and I think that it looks good now.

  • catch committed 46b3fa5 on 8.4.x 
    Issue #2850691 by claudiu.cristea, hchonov: Deprecate TermInterface::...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

Oh, this was so fast :)

Status: Fixed » Closed (fixed)

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

For the record, this method was added on purpose, just like Node::getType() and was requested in the initial entity field api conversions by @webchick and others.