ChromaticHQ / civilcomments

Drupal module for the Civil Comments platform.
https://www.drupal.org/project/civilcomments
0 stars 0 forks source link

3 create custom field type for module #4

Closed ctorgalson closed 8 years ago

ctorgalson commented 8 years ago

This PR adds a custom field type, widget, formatter and template. This makes the following workflow possible:

The contents of the template--essentially just a div and a script element--are successfully rendered when the field is set to 1 or 2, and the entire field is considered empty and not rendered when the value is 0.

There are no frills, no other options at this point. Consider it a proof of concept.

mmatsoo commented 8 years ago

@ctorgalson The only thing I can think to ask about are the t() functions in CivilComments.php. I have gotten the impression that using $this->t() is preferred, but not having tested, I don't know if it can just be dropped in.

From https://api.drupal.org/api/drupal/core%21includes%21bootstrap.inc/function/t/8

When possible, use the \Drupal\Core\StringTranslation\StringTranslationTrait $this->t(). Otherwise create a new \Drupal\Core\StringTranslation\TranslatableMarkup object directly.

Again, I don't know if this is appropriate here, but it's the only thing I noticed!

ctorgalson commented 8 years ago

@mmatsoo: I saw that and took the lazy way since there were core modules I looked at for info still using t(). I'll change it.

ctorgalson commented 8 years ago

@mmatsoo, and anyone else following along. This turns out to be kind of interesting.

So in CivilCommentsDefaultWidget.php, I had previously been inconsistent about using t() versus $this->t().

In that file $this->t() was available, even without adding use Drupal\Core\StringTranslation\StringTranslationTrait;, because my CivilCommentsDefaultWidget class is a grandchild of PluginBase which includes use StringTranslationTrait;.

On the other hand, in CivilComments.php (where I'd borrowed code from another module) I was just using t() directly.

In this instance, it's not possible to use StringTranslationTrait and access $this->t() because CivilComments::propertyDefinitions() is a static function:

Fatal error: Using $this when not in object context...

That means that in this case, the alternatives are

  1. Use t(), or
  2. "[C]reate a new \Drupal\Core\StringTranslation\TranslatableMarkup object directly".

I'm sticking with option 1 :)

mmatsoo commented 8 years ago

@ctorgalson Nice explanation. :cool:

AlannaBurke commented 8 years ago

Looks good!