Per #2393339-42: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality, the user signature field in Drupal core is not a "true" Field API field, and is instead kind of a strange base field that has been brought along since the Drupal 4.x days. Because of this, it represents a (small) security risk, since a site may choose to implement field access for user signatures and expect that to work. i.e. you might not want to show signatures to anonymous users to prevent search engines seeing them, or due to privacy concerns.

Given that this makes fixing that problem critical, and given that user signatures are an optional feature that has not received proper maintenance in at least a couple of core releases, there is strong consensus, including from the original author (@webchick) and Forum module maintainer (@larowlan), to move user signature functionality to contrib instead, so that is what the patch does.

@andypost has created a start at what signatures might look like as a separate module at https://github.com/andypost/signature and the Dealing with unsupported (abandoned) projects is being followed for the Signature module in contrib: #2457017: Offering to maintain Signature for 8.x.


Original issue summary by @catch:

User signatures are hard-coded in the user table schema, seems like an obvious candidate for a field.

Part of me thinks it could even move to comment module, but could probably just start with the conversion before worrying about ownership.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

I agree we should convert it first.

But since you already started the owner discussion ;) I actually think that user signatures are only ever used for forums (and the utter ugly code for it pretty much dates all the way back to the introduction of forum). However, forum is use-case specific already, and I could see potential other use-cases for user signatures. What matters is that user signatures are not simply text fields -- they additionally have business logic applied to them. Thus, I'd almost go with a user_signature field module.

Michelle’s picture

When you do start worrying about ownership, I suggest not putting it in comment module considering adding a signature to a node is a perfectly valid use case. :)

adammalone’s picture

Assigned: Unassigned » adammalone

Field-ifying signatures sounds like a pretty good idea. Then it can be applied to nodes, comments whatever.

I'm going to assign this to me to have a look over the next couple of weeks.

sun’s picture

Title: Make the user signature a field » Convert user signature into a normal field

Clarifying issue title. This goes hand in hand with #569434: Remove taxonomy term description field; provide description field for forum taxonomy

I'm currently trying to make Drupal 8 use an alternative user storage under the hood, and all of these additional + unnecessary user-preference-alike settings in the {users} table really get in my way. :-/

Really, anything would be more appropriate than the currently hard-coded table column... even UserData would be more appropriate.

However, a text field attached to the User entity would be the most appropriate approach, as it contains all the necessary logic and plumbing already.

It also removes a fairly obscure checkbox/setting from the administrative Account settings page, which happens to be the trigger for the entire functionality (and only god knows what happens when you enable or disable that checkbox).

Like #1292470: Convert user pictures to Image Field, this will result in the removal of quite some legacy code and is a much needed API clean-up.

sun’s picture

adammalone’s picture

I've followed the example of #1292470: Convert user pictures to Image Field to make signatures their own field. I've decided to give 'ownership' to the user module as the signatures are fields on the user entity.

Hopefully I've gone down the right track with this - any tests relying on the old signature system will not work as I've not altered them so we'll call this a first attempt.

I would probably include a caveat that signature fields should be displayed as 'trimmed' by default rather than free text for however long the user wants.

adammalone’s picture

Status: Active » Needs review
Grayside’s picture

+++ b/core/modules/comment/comment.moduleundefined
@@ -1641,16 +1641,16 @@ function template_preprocess_comment(&$variables) {
+    $variables['user_picture'] = user_view($account, 'compact')['user_picture'];
...
+    $variables['user_signature'] = user_view($account, 'compact')['user_signature'];

Direct array dereferencing depends on PHP 5.4.

adammalone’s picture

Thanks Grayside - wasn't aware of that.

I was trying to think of the best way to accomplish what was desired. If it is just left at
user_view($account, 'compact') then all of the fields in the compact view of the user entity are displayed. This means that picture & signature are displayed twice. (This seems to be a bug from when user picture was fieldified).

Perhaps a better way would be to:

  $user_view = user_view($account, 'compact');
  if (theme_get_setting('toggle_comment_user_picture')) {
    $variables['user_picture'] = $user_view['user_picture'];
  }
  else {
    $variables['user_picture'] = array();
  }
  if (theme_get_setting('toggle_comment_user_signature')) {
    $variables['user_signature'] = $user_view['user_signature'];
  }
  else {
    $variables['user_signature'] = array();
  }

Status: Needs review » Needs work

The last submitted patch, 1548204-6-fieldify_signatures.patch, failed testing.

sun’s picture

If we keep the field in User module for now, then I think we want to add a new view mode/display 'signature', which only renders that text field.

Please also note that #1853378: Optimize user_view('compact') in template_preprocess_node() is about to change how the compact view mode is processed and rendered.

adammalone’s picture

I can reroll the patch as and when that happens in conjunction with amending to code in #9.

Should also take into account that displaying the user picture via the methods it currently does is sort of broken as it just renders everything in the 'compact' view of the user.

jibran’s picture

Status: Needs work » Needs review
Issue tags: -Entity system, -API change, -API clean-up, -Field system

#6: 1548204-6-fieldify_signatures.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +API change, +API clean-up, +Field system

The last submitted patch, 1548204-6-fieldify_signatures.patch, failed testing.

jibran’s picture

Issue tags: +Needs reroll

It needs reroll.

amateescu’s picture

Issue summary: View changes
Status: Needs work » Postponed
Issue tags: -Needs reroll
FileSize
8.66 KB

In light of #2142549: Support multi-column field types in base tables, the signature can stay as a base field and be a regular 'text' field instead of two string fields stiched together. Here's an initial patch that depends on the above, which means we'll have to wait for that to get in before moving forward here.

klonos’s picture

Status: Postponed » Needs review
sun’s picture

Thanks for the alternative approach, @amateescu.

But, hm. One of the secondary goals envisioned here was to get rid of the columns in the {users} table (cf. #4). If you do not need or use user signatures on your site, then you should not see the properties, data, schema columns, etc.

By turning the user signature into a field, the entire storage and data would only exist if you actually use user signatures on your site. Otherwise, they wouldn't appear anywhere.

Thoughts?

amateescu’s picture

The thing is.. I didn't read the comments in this issue before posting. I was told in #2142549: Support multi-column field types in base tables that user signatures would serve as a good testcase, so I just extracted the work from that patch and posted here.

If the goal is to not have the data in {users} anymore, then sure, a configurable text field makes more sense, so feel free to disregard the patch in #16 :)

Berdir’s picture

Status: Needs review » Needs work

I suggested this approach because of the way the signature field is used. It's not displayed on users, it's pulled in on comments and displayed there. That is something that requires custom code and doesn't work well in combination with a custom field. We'd need to have that code in the profile or so or some other place where it's conditional.

Converting it to such a field at least means less custom code and standard features, like edit.module's embeded field usage tracking would work for the signature as well.

Needs a re-roll now anyway.

webchick’s picture

Just to throw another potential wrench in the works... Signatures should ideally be able to optionally be displayed on nodes, too. At least that's how every forum software on the planet works, which is the origin of this feature.

This is the one part of my never-ending quest to fix signatures I never got to. ;( But even if we don't fix this in core, contrib will definitely need to be able to do this, so whatever approach is taken here should factor that in.

sun’s picture

@Berdir, I fully agree that the base field approach should be using the new standard field facilities in D8. We can perfectly do that as a first step, although I wonder whether we shouldn't create a separate issue for doing so?

That is, because this issue always was about the hard-coded columns in the {users} table. As mentioned in #4, I attempted to replace the user account storage with an external system (as a quick experiment, I used the {users} datatable of a Drupal 6 site) some time ago.

To do so, I swapped out the user storage controller with my own, which worked like a charm, but having to care for signature* values really came unexpected, since signatures do not really have any business in there.

I agree that a custom field might be hard to maintain in a reliable way. Perhaps we should simply move the functionality into a new [user_]signature.module with corresponding database table, and simply expose it as an extra field?

That approach would inherently resolve @webchick's note on making the signature available for (forum) nodes, too.

Thoughts?

sun’s picture

Assigned: adammalone » Unassigned
Status: Needs work » Needs review
FileSize
7.57 KB

For now, just starting a straight re-roll of the last patch.

Next, I'm going to explore how hard it is to move the functionality into a new user_signature.module.

Status: Needs review » Needs work

The last submitted patch, 24: user-signature.24.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

24: user-signature.24.patch queued for re-testing.

Berdir’s picture

Status: Needs work » Needs review

24: user-signature.24.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 24: user-signature.24.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
28.12 KB
26.27 KB

Here we go. This is my first more substantial dive into the new Entity Field API of D8, so please don't shoot me in case I missed something big or super-obvious. ;)

As a random benchmark: The proposed changes would have taken me ~20 minutes in D7. The total RTT for this patch was 4h.

I primarily had a lot of DX related problems with (1) learning new entity/form code execution flows, (2) deciphering which classes and services are actually getting invoked, (3) reverse-mapping arbitrary string IDs to plugin/service/class implementations, (4) making sense of entity/property/field/data/definition thingies that seemingly appear to be mixed and mashed and used like crazy :P, and lastly, (5) trying to implement seemingly basic things that do not seem to be cleanly supported yet ;)

All in all, things appear to make sense, but I did not have the impression that the system is prepared for very typical bridge/glue modules like this yet. (?)

I don't know whether this patch is correct in any way, but that's how I'd interpret the current system architecture as a "newbie". Hopefully this helps us to learn some more about how we can improve the Entity/Field API experience for developers. I'd certainly love to learn! :)

What do you think?

The last submitted patch, 29: user.signature.29.patch, failed testing.

Berdir’s picture

Some random feedback on @todo's and questions in the code.

  1. +++ b/core/modules/user_signature/user_signature.module
    @@ -0,0 +1,253 @@
    +  }
    +  // @todo There does not appear to be a way to cleanly load the field item
    +  //   values of attached third-party fields within the Entity API or
    +  //   Entity Field API scope? -- hook_entity_load() is too gross (performance),
    +  //   and no lazy-loading in FieldItemBase?
    +  $signature = db_query('SELECT signature__value AS value, signature__format AS format FROM {user_signature} WHERE uid = :uid', array(':uid' => $account->id()))->fetchAssoc();
    +  if ($signature) {
    +    $account->set('user_signature', $signature);
    +  }
    

    Not like you'd expect it to. Use the load hook, that will become part of the cached entity values (not right now in the entity caching patch, but I want to get there), so you shouldn't have to worry about performance too much, It's actually likely faster to get it like that from the cache than setting like like this, because this requires instantiated field objects.

    The alternative is computed properties but I don't think that makes much sense here.

  2. +++ b/core/modules/user_signature/user_signature.module
    @@ -0,0 +1,253 @@
    +    // @todo Wasn't #maxlength support for #type 'textarea' committed already?
    +    '#maxlength' => $account->get('user_signature')->getFieldDefinition()->getSetting('max_length'),
    

    You should be able to get to the definition directly with with $account->getPropertyDefinition('user_signature').

    The property/field mismatch will be solved in one of the typed data issues I think.

    Also, the whole form element should just be a text item widget, similar to how node title now works. There are still multiple issues related to make DX better, but that's where it's going afaik.

  3. +++ b/core/modules/user_signature/user_signature.module
    @@ -0,0 +1,253 @@
    + *   enforced?
    ...
    + * @todo Why is the TextFieldItem 'max_length' constraint not automatically
    

    Because validations currently only work for configurable fields, see the #2002180: Entity forms skip validation of fields that are edited without widgets.

  4. +++ b/core/modules/user_signature/user_signature.module
    @@ -0,0 +1,253 @@
    +    }
    ...
    + *
    + * @todo Move CRUD logic into new user_signature FieldItem extends TextFieldItem?
    + */
    +function user_signature_user_form_submit($form, &$form_state) {
    +  $account = $form_state['controller']->getEntity();
    +  if ($account->id() && isset($form_state['values']['user_signature'])) {
    +    if (!$account->get('user_signature')->isEmpty()) {
    +      db_merge('user_signature')
    +        ->key(array('uid' => $account->id()))
    +        ->fields(array(
    +          'signature__value' => $account->get('user_signature')->value,
    +          'signature__format' => $account->get('user_signature')->format,
    +        ))
    +        ->execute();
    +    }
    +    else {
    +      db_delete('user_signature')
    +        ->condition('uid', $account->id())
    +        ->execute();
    ...
    +  }
    

    You've chosen a rather strange middle path by making it an entity field and still have custom storage/logic.

    Unfortunately, entity storage doesn't support non-configurable fields yet, see #2144263: Decouple entity field storage from configurable fields. With that, it should be able to take care of storing your values.

    For now, you could simply define it as a configurable fields by throwing the necessary config files into your config folder. You'd just need to take care of the entity form display I think.

    Or do it the other way, and don't bother with entity fields at all. Because right now, you get almost nothing from doing so :)

  5. +++ b/core/modules/user_signature/user_signature.module
    @@ -0,0 +1,253 @@
    +  // Fetch signatures for all authors, keyed by entity ID.
    +  $uids = array();
    +  foreach ($entities as $entity) {
    +    $uids[$getAuthorId($entity)] = $getAuthorId($entity);
    +  }
    

    I don't really get why using an anonymous function is any better than just defining it as a normal function :)

yched’s picture

+ * @todo Why is the TextFieldItem 'max_length' constraint not automatically
Because validations currently only work for configurable fields, see the #2002180: Embrace entity validation in entity forms .

More precisely, field constraints are only validated for fields rendered by Widgets rather by custom FAPI code, because Widgets can map violation back to a FAPI structure, and arbitrary FAPI code can't.
If you don't use widgets, you're on your own for FAPI handling, including validation...

It is currently possible but fairly convoluted to have a base field use a widget, #2144919: Allow widgets and formatters for base fields to be configured in Field UI is the issue about making it simple.

sun’s picture

FileSize
30.94 KB
15.11 KB

Thanks for the helpful insights and pointers!

Re-implemented user signature as a field type attached to User entity.

The underlying idea: The user signature belongs to the User entity, and content entities like Comment and Node are loading the User entity of the author already for theming.

The last submitted patch, 33: user.signature.33.patch, failed testing.

sun’s picture

andypost’s picture

I wonder that there's a whole module for that? why not just and a few config files to create field?

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
    @@ -26,6 +26,33 @@
    +  public function getAuthor();
    ...
    +  public function getAuthorId();
    ...
    +  public function setAuthorId($uid);
    
    +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -338,6 +338,28 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
    +  public function getAuthor() {
    ...
    +  public function getAuthorId() {
    ...
    +  public function setAuthorId($uid) {
    

    Happends in #2028025: Expand CommentInterface to provide methods

  2. +++ b/core/modules/user_signature/user_signature.install
    @@ -0,0 +1,45 @@
    +/**
    + * Implements hook_uninstall().
    + */
    +function user_signature_uninstall() {
    +}
    +
    

    is not needed

tstoeckler’s picture

Expanding on #36 I think it would be more consistent with what we do for the user picture. There you just configure the 'compact' view mode however you like. We could add a 'signature' view mode and then add the signature field as simple config.

tstoeckler’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
    @@ -26,6 +26,33 @@
    +  public function getAuthor();
    ...
    +  public function getAuthorId();
    ...
    +  public function setAuthorId($uid);
    
    +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -338,6 +338,28 @@ public static function postDelete(EntityStorageControllerInterface $storage_cont
    +  public function getAuthor() {
    ...
    +  public function getAuthorId() {
    ...
    +  public function setAuthorId($uid) {
    

    Let's go with Owner here as per #2078387: Add an EntityOwnerInterface

  2. +++ b/core/modules/user_signature/lib/Drupal/user_signature/Plugin/Field/FieldType/UserSignatureItem.php
    @@ -0,0 +1,91 @@
    + *   conditionally define field schema based on field definition properties.
    ...
    + * @todo Consolidate TextItem and TextLongItem into a single field type and
    

    I'm not sure how that could really be done, but it sounds interesting. Let's open an issue for that.

  3. +++ b/core/modules/user_signature/user_signature.install
    @@ -0,0 +1,45 @@
    +/**
    + * Implements hook_uninstall().
    + */
    +function user_signature_uninstall() {
    +}
    

    ?

  4. +++ b/core/modules/user_signature/user_signature.module
    @@ -0,0 +1,187 @@
    +  // Make sure the signature isn't longer than the size of the database field.
    ...
    +  $maxlength = $schema['fields']['signature__value']['length'];
    ...
    +  $schema = drupal_get_schema('user_signature');
    ...
    +      'max_length' => $maxlength,
    ...
    +    ->setSettings(array(
    ...
    +      'text_processing' => 1,
    +    ));
    

    I don't see a reason for this indirection. Can this not be validation constraint on the field item directly?

jibran’s picture

Status: Needs review » Needs work
mgifford’s picture

Berdir’s picture

I think it is way too late to move it to a separate module (even though that would actually work now, and a lot easier than done here). The only thing we might still have a chance is to convert it to a text field type. That would still mean a storage change, resulting in signature__value and signature__format as column names and a single field. But even that might be too late, can't say.

jibran’s picture

fago’s picture

The only thing we might still have a chance is to convert it to a text field type. That would still mean a storage change, resulting in signature__value and signature__format as column names and a single field. But even that might be too late, can't say.

Agreed. That is the least disruptive change right now and does not have to change templates or so (just disable display configuriablity)- but would help us solving #2405943: User entity validation misses form validation logic. At least we could leave the existing getter() in place?

It poses a new problem though: User module would have depend on filter module for the filter format. (Right now, filter format depends on it.)

jibran’s picture

It blocks #2425193: Convert the signature field in user_field_data to the ordinary views field which is a child issue of critical so should we change this to critical?

jhodgdon’s picture

Priority: Normal » Critical

I've closed #2425193: Convert the signature field in user_field_data to the ordinary views field as a duplicate of this issue, because (a) this seems to be the Right Way to fix the problem of making the signature field actually translatable and access controlled in Views and (b) it may not only be the Right Way but probably is the Only Way.

This one now becomes a child of #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality and becomes critical.

catch’s picture

Issue tags: +D8 upgrade path
Berdir’s picture

Not convinced about B). As commented above, we have a non-trivial cross-dependency here between user and filter module if we make it a base field.

I'd argue that in the current state, the "right way" is not always the way we can still do...

catch’s picture

This issue was originally about making the signature a field API field - because it's very site-specific whether you want signatures or not (and we have a configuration option in core to disable them in the UI).

Berdir’s picture

Right. So what would we do with it exactly? And who would create it?

We already have something like that for the profile picture.. which is configuration, but we also have API functions that assume that *if* it exists, then with a specific name and so on..

jhodgdon’s picture

There is some kludgy code already in the User module that disables the Signature field if the Filter module is not enabled. See for instance user_modules_uninstalled() in user.module, and I think there's code for this elsewhere as well, just cannot remember where.

Probably we'd need to do something similar with this field, but ... wouldn't a field that depends on Filter just automatically not get its config imported until all the dependencies are present? That seems like a cleaner thing than the kludge that is there now.

catch’s picture

but we also have API functions that assume that *if* it exists, then with a specific name and so on..

I think those are mostly dead code now? User picture rendering on comments just uses the 'compact' user view mode - this gets turned into a 'user_picture' variable but has no dependency (optional or otherwise) on the field.

I see user_picture_enabled() in user.module and a reference in theme settings but wondering whether that's just never cleared up since 7.x?

So for signatures we could probably add a 'signature' view mode for users, and use this in comments, and add the field in standard.profile (and migrations) not much else?

Berdir’s picture

You can have optional config like that in HEAD, but #2090115: Don't install a module when its default configuration has unmet dependencies makes that possible.

If we make it either an optional field in user or part of standard.install, then yes, that might work.

The interesting thing is that standard actually doesn't have that feature enabled, so we'd change it to be enabled by default for standard? If not, then we would essentially drop the feature completely? Or make the UI checkbox create the field or something.

yched’s picture

Or make the UI checkbox create the field or something

IIRC so far we have avoided "some checkbox somewhere creates a field somewhere else" approaches, because of duplicate UIs and the difficulty of enforcing they stay in sync.
I guess that's easier in a wizard-create UI that you don't ever see again once the "thing" has been created, but having two separate UI screens control the same thing is tricky.

andypost’s picture

The same way we have #2230177: Without Field UI comment module presents a poor UX unsolved
Also I think that loading user could cause access checks for filter so circular deps too

larowlan’s picture

Back to comment 1 - this seems to be forum specific? Why not make it a field in forum module's default config?

dawehner’s picture

What about exposing the signature field as part of a dedicated module. Forum module could depend on it, but I think signature is really not a common used
feature, compared to the generic usecase of the user module.

andypost’s picture

doing a reroll for separate module, also I think we need to expose signatures through post render so changing a signature should not invalidate all entities where used.

About settings, there is related issues

effulgentsia’s picture

also I think we need to expose signatures through post render so changing a signature should not invalidate all entities where used

That might be a really good idea, but I think it's out of scope for this issue. Can we defer that to a noncritical followup?

cosmicdreams’s picture

I like #55 and #56 because I don't want user signatures on my sites. If I really wanted users to have signatures on their posts I'd extend the user entity to provide it.

Berdir’s picture

doing a reroll for separate module, also I think we need to expose signatures through post render so changing a signature should not invalidate all entities where used.

Not convinced by that at all, actually :)

Comments and nodes have the user cache tag anyway, so they will be invalidated anyway when the user entity is saved.

post_render isn't free, it requires additional rendering on every page load.

Wim Leers’s picture

Not convinced by that at all, actually :)

Comments and nodes have the user cache tag anyway, so they will be invalidated anyway when the user entity is saved.

post_render isn't free, it requires additional rendering on every page load.

Exactly.

Using #post_render_cache for every single user signature would mean that on a forum with e.g. 50 forum posts visible at a time, by 50 different users, we'd be rendering and replacing 50 different placeholders. User signatures usually don't change very often, so I don't see why we'd want to do that.

webchick’s picture

Just one small note, since we have to do some amount of wrangling either way, it would be amazing if whatever is done here is done in such a way that signatures can be displayed on forum nodes as well as their comments. That is forum functionality 101 and has been a thorn in my side since Drupal 4.6. ;)

Wim Leers’s picture

#62: Hah! If it's a field, that should be trivial nowadays? Comments and nodes are entities, and they both have authors. So however this works exactly; it'd just be a matter of configuring the user field formatter on both the node bundle and the comment bundle's view displays.
Of course, I don't know anything about how this patch actually works, so that could be total nonsense.

larowlan’s picture

As maintainer of forum, my personal preference is to remove this from core. Wholesale.
The word signature does not appear in forum.module at all, so it's presence would be something provided by user as it stands, which would suggest that a contrib module could easily add it back without much trouble on behalf of forum.
I can't see any situation where we get close to an RC and say 'hey we can't ship 8.0.0-rc1 because user signatures'.
Who's with me?

/me ducks for rain of abuse

Berdir’s picture

Not quite like that, a view mode can't be split up later and be split into multiple places. We can only do that if we render multiple fields explicitly, the way views does that now/soon, but then we lose the configurability.

That said, there's no reason we could do the same for nodes that we do on comments, but not sure if we should that here.

Wim Leers’s picture

#64: heh! :P

I personally always hated forum signatures, on every forum I've ever been on (at least a dozen), in all the possible implementations I've ever seen (several dozen). Endlessly repeated annoying text? That's what a user *profile* page is for. This is a remnant of e-mail signatures (which are also annoying as hell), and just made it into web forum software because e-mail lists were the original forums.

+1

cosmicdreams’s picture

What prevents a user signature from being a textarea (text, formatted, long) and inform the user reference formatter field how to handle it if present?

andypost’s picture

Looking at 2 first questions
http://drupal.stackexchange.com/questions/101118/signature-user-field-no...
http://drupal.stackexchange.com/questions/115275/how-to-not-show-user-si...

I'm really +1 to remove signatures to contrib

Also there's a issue to clean-up comment entity templates so at least template_preprocess_comment usage of user_view() could be fixed.

About post render, I'm sure that more often a forum discussions used by only few users, so 50 comments could be done by 5 users with only 2 with signatures.

PS: Still trying to separate signatures to optional module

andypost’s picture

Status: Needs work » Needs review
FileSize
25.08 KB

Removal of mentions, let's see how much broken

Migrate dump and variables still define the source

TODO:
1) \Drupal\Component\Utility\NestedArray doc block uses example about signatures
2) Fix CHANGELOG.txt

Wim Leers’s picture

Also there's a issue to clean-up comment entity templates so at least template_preprocess_comment usage of user_view() could be fixed.

Can you point to that issue? I'd love to review it.

Status: Needs review » Needs work

The last submitted patch, 69: 1548204-signature-69-removal.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
8.47 KB
33.34 KB

Remove remains and fix tests, core/modules/user/src/Tests/UserSignatureTest.php is removed also

Status: Needs review » Needs work

The last submitted patch, 72: 1548204-signature-71-removal.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
3.14 KB
36.49 KB

And more clean-up

andypost’s picture

Patch is green, so now we need to decide on module name, my +1 to comment_signature

dawehner’s picture

Given that @webchick thought about signatures both on comments and the node itself, I would like to suggest user_signature

Michelle’s picture

I think user_signature is better as well since it is tied to an individual user and could, in theory, be used anywhere.

adammalone’s picture

+1 user_signature

jhodgdon’s picture

Thank you for updating the hook_help() implementation along with this patch! I have no opinion on the merits of the patch, but updating the help at the same time is much appreciated. :)

xjm’s picture

Priority: Critical » Major
Issue tags: -Needs Drupal 8 critical triage

So following up on #44 - #45, the case for the meta #2393339: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality being critical was that we would leave it critical only until such time as we had identified the scope of the critical work for that issue, and made sure that any specific issues were filed. Children are not necessarily critical (and in many cases won't be). This issue by itself is not critical, so downgrading.

xjm’s picture

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

and made sure that any specific issues were filed.

                          .'  .
                         .'.'.' .
                        .`.'.`'.
                       ..'..'.`'
                       `,'....`
                      `' ..'`.
                     `'``''`.
                     .'.``'`
                  .'`..'''`.
                    ````.'`
                  xl""``""lx
                 X8Xxx..xxX8X
                 8X8bdX8bd8X8
                dX8Xbd8XbdX8Xb
               dX8Xbd8X8XbdX8Xb
              dX8Xbd8X8X8XbdX8Xb
            .dX8Xbd8X8X8X8XbdX8Xb.
          .d8X8Xbd8X8X8X8X8XbdX8X8b.
      _.-dX8X8Xbd8X8X8X8X8X8XdbX8X8Xb-._
   .-d8X8X8X8bdX8X8X8X8X8X8X8X8db8X8X8X8b-.
.-d8X8X8X8X8bdX8X8X8X8X8X8X8X8X8db8X8X8-RG-b-.

No, there is still tons of unknown work in front of us

Need to be checked to see if they can be removed/replaced.

The patch is looking great for me, lets get better that feature removed earlier than later and make a good contrib module.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 74: 1548204-signature-74-removal.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
38.83 KB

A bit more clean-up in migrations
Suppose to get review for the hunks in \Drupal\migrate_drupal\Plugin\migrate\load\LoadEntity::processTextField() - probably that code would be needed for d7 migrations

jibran’s picture

Title: Convert user signature into a normal field » Remove user signature and move it to contrib

Let's be real.

jhodgdon’s picture

Thanks for the updated title. Also needs update in issue summary and change notice, since this is functionality that was in 7 and in 8 until now, and is proposing to be removed.

However... Is anyone actually proposing to do this in a contrib module? And how? Is it even feasible to have it not kludged into the user and comment modules like it is now? And if it is feasible, why can't it be moved to a core module? I mean... This functionality is clearly an ugly kludge, and I agree that removing the code feels good. But if the only way to provide the functionality is through this ugly kludge, I am not sure removing it is the right fix.

catch’s picture

Assigned: andypost » webchick
Priority: Major » Critical
Issue tags: +Needs Drupal 8 critical triage

We have the 'compact' view mode for users, which is used in core for user pictures (although confusingly there is still a user picture theme setting which is close to redundant).

You could easily add the signature to the 'compact' user view mode, then the signature would be shown next to the picture on comments, some forums show this (i.e. particularly two-column comment templates etc.). Shouldn't require any hacks. Rendering it elsewhere in the comment template would require some work but again ought to be OK.

Per #2393339-42: [META] Make sure Views base fields are using Field API for formatting, and do not lose functionality I do think this is critical - can 100% see a site implementing field access for user signatures and expecting that to work. i.e. you might not want to show signatures to anonymous users to prevent search engines seeing them, or due to privacy concerns. Tagging for triage though.

Also assigning to webchick for the 'remove from core' angle. I do think that's the right choice since this is an increasingly marginal feature.

Michelle’s picture

re #86: This might be a crazy idea but I was thinking in contrib it could be a field. The field "data" would be any settings that you might want to set per instance rather than the signature itself. Then the display of the field would pull in the signature. That way, it could be easily added to any fieldable entity.

jhodgdon’s picture

@Michelle: I think the suggestion in #89, if I'm understanding it correctly, is basically:
- Signature is a field on the User entity.
- It gets added to the "compact" view mode for the User.
- This view mode is what is already being shown along with user picture for comments, so signature would be included in that.

I'm not entirely convinced you want the signature placed near the picture and/or user name (picture is often upper left corner, user name across the top, signature at the bottom), but something like this does seem like a reasonable solution other than that detail.

Michelle’s picture

I think I jumped in without thinking it through enough. I was thinking about the _display_ of the signature and making that a field that could be added to comments, nodes, users, whatever. Basically it would take in the author of the entity the field is on and spit out a signature based on settings on the field. I wasn't thinking at all of where the actual signature would be stored. So I guess there would end up being a "signature field" on the user that holds the signature and a "signature display" field that could be put anywhere, which maybe doesn't make a whole lot of sense since that isn't how fields are normally used.

webchick’s picture

Thanks for assigning to me. This is near and dear to my hearty since this was the whole reason I got into core dev. ;)

Like jhodgdon and Michelle I would feel a lot better about this if a workable direction was given to a prospective contrib module maintainer that met the following requirements:

- field in user profile
- updating it there updates it everywhere it is displayed
- displayed at the *bottom* (under the user picture is definitely not what people would expect and drupal behaved this way for at least 10 years) of both comments and nodes
- you can configure which node/comment types so it's shown on so for example, only forum posts/replies

Right now that direction feels basically like:

¯\_(ツ)_/¯

If lots of smart core devs can't figure this out, I'm doubtful contrib is just going to magically make it happen, and then sites lose data (and in this case highly personal data -- as in "that's part of my identity") when they move to D8.

One question is whether another way to resolve the critical is simply to remove views integration for the field.

jhodgdon’s picture

So... On #2425193: Convert the signature field in user_field_data to the ordinary views field I attempted to fix the critical part of this by adding a proper formatter to the Signature base field (making it a proper long text field with a format, and then you could just use the Field API formatter in Views for Signature). This failed, because base entity fields can't be long-text-with-format (we don't want them to depend on Filter), and exposed to me the general kludgyness of Signature in the User module (the "field" gets turned off if the Filter module isn't there, to avoid having User depend on Filter, etc.). Plus there's a "use signatures" setting on user accounts, which is really also a kludge.

To me, this says that the Signature field should be a Field UI field, not a base entity field, so that (a) it can really be long-text-with-format, and (b) it can be turned off by removing the field. The kind-of-and-kludgy way it's being done now also probably doesn't properly support translation of user signatures anyway using the Entity Translate stuff. If both User and Filter are turned on, this field config would be imported and you'd have signatures; if Filter is missing you wouldn't get the field config, because it depends on Filter.

But then there's all the other kludgy special stuff that Signature does to make it appear magically at the bottom of forum comments or whatever else it does. I haven't delved into that enough to know how that works, but it's probably magic. I mean, there's a relationship between a comment and the user who created it, but in rendering an entity with an entity relationship, you can't normally render the related entity scattered around the way Comment is doing: user name one place, picture a second place, signature a third place. So this is problematic: both Picture and Signature get special treatment and neither of them is a proper Field.

Anyway. You all probably knew all of this... but it *really* makes me doubt this can go into Contrib easily.

Berdir’s picture

I really don't see a problem with moving this to contrib.

@jhodgdon:
- There is no problem with having a text field as a base field. Taxonomy terms do that for the description, it works fine (although I think that description should be a configurable field, but that's a different problem). The problem is having a text base field on *users*, because user is a required module and text and filter is not.
- There's not as much magic as you think (depends on what you understand as magic). It's built in the comment view builder, added to comment templates and printed there.

What makes it complicated is that user signatures are trying so hard to be implemented in a Drupal 5 way in Drupal 8 ;)

Here's what a contrib field has to do:
- Expose an extra field "User signature" on comments and nodes (or every other entitiy type that you want to, because there's nothing special about that) (Instant and free configurability per bundle and view mode)
- A bit of code that adds *something* into that extra field. It could either be a single text field that the module provides, or it could be the user entity in a given view mode (the existing compact mode doesn't work, it is not possible to render an entity and then split it up due to render caching).

That being in a separate module means:
- No custom configuration required, if you don't want the feature, don't enable the module, and you can configure it to be shown where you want, no custom template/preprocess logic necessary by default. (Unless you want to, which is also possible, of course)
- No overhead for the 99% of the sites that don't want user signatures
- Free and working views integration

We could have done this any time in the last 4 years in D8 development, but we didn't, because no active core developer cares about user signatures. Which makes it a good candidate for contrib IMHO. I'd suggest to revive https://www.drupal.org/project/signature, by the way..

andypost’s picture

Created a minimal module to display signatures in comments https://github.com/andypost/signature

+1 to use https://www.drupal.org/project/signature that will require some clean-up on names in the module.
Seem only thing is left to find a new maintainer for the module

andypost’s picture

FileSize
38.83 KB

reroll

jhodgdon’s picture

Thanks for the explanation Berdir and the minimal module andypost. Can we get all of this stuff into the issue summary and a draft change notice, since there now seems to be (a) agreement that moving to contrib is a good idea and (b) demonstration of it actually working and (c) a patch to remove signature from core?

I was going to add the tags for change record and summary, but they're already present. ;)

netturbina’s picture

Yes, moving to contrib is a good idea. #93 100% right.

webchick’s picture

Assigned: webchick » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Played with andypost's code and though it needs some work, it definitely does the gist of what the core version does. Thanks a lot for that.

Updated the issue summary. Also filed #2457017: Offering to maintain Signature for 8.x, unless there are other people dying to take it over. :P I pinged ultraBoy and jonaswouters via their D.o contact forms as well, per https://www.drupal.org/node/251466.

Marking RTBC, though I'd still love an answer to this:

One question is whether another way to resolve the critical is simply to remove views integration for the field.

...because the parent issue makes me extremely nervous as a critical. If there's any way of downgrading it to major by removing Views integration from these "rogue" fields and adding it back later once "major" fixes for these other issues land, that would be awesome.

andypost’s picture

Also this issue will open a way for #2031883: Markup for: comment.html.twig and #2002094: Improve performance of comment.html.twig to make comment templates and rendering faster and cleaner. Also this helps to #2428509: Comment links weight should be managed by view display so comment links and other user-defined fields could be reordered through field UI.
Yes, new module needs some love especially in access/translation cos we have no multilingual support for extra fields.

If there's any way of downgrading it to major by removing Views integration from these "rogue" fields and adding it back later once "major" fixes for these other issues land

I see bigger problem with comment templates because I'm sure that people more often will fight with templates/preproces wtf then use signature in views.

webchick’s picture

Ok, https://www.drupal.org/project/issues/signature is now looking more lively.

This still needs a change record.

I don't think Dries needs to sign off on this, but I'll be talking to him tomorrow so will commit it then, as long as the CR is ready.

andypost’s picture

andypost’s picture

Issue tags: -Needs change record
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Really sorry, yesterday got away from me.

Committed and pushed to 8.0.x. Thanks! So long to 4 years of work, sniff. ;)

andypost’s picture

looks not pushed

alexpott’s picture

On behalf of @webchick I committed 7d31cc8 and pushed to 8.0.x. Thanks!

  • alexpott committed 7d31cc8 on 8.0.x
    Issue #1548204 by andypost, sun, typhonius, amateescu: Remove user...
YesCT’s picture

Maybe this change record should have been updated instead? (or in addition)
https://www.drupal.org/node/2116417
"Modules and themes removed from core in Drupal 8"

YesCT’s picture

OH, this wasn't a module removed from core.. it was functionality that is to be replaced by contrib. Hm. I dont know if we have a group of things like that.

andypost’s picture

@YesCT that's why I ask in #101, maybe we just add new section to it with "functionality moved to contrib"? like layout module...

jhodgdon’s picture

It seems like it would not hurt at all to add a note there!

andypost’s picture

xjm’s picture

Status: Fixed » Closed (fixed)

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