The module works great, however can there be a feature to name the tokens. This really helps us in tracking to whom we have shared the token with and what token to expire / delete in case required.

CommentFileSizeAuthor
#12 3144610-12.patch6.61 KBshaktik
#10 3144610-10.patch6.65 KBshaktik
#8 3144610-8.patch5.25 KBshaktik
#5 3144610-5.patch4.19 KBshaktik
#2 3144610-2.patch8.16 KBshaktik

Issue fork access_unpublished-3144610

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

vaibhavjain created an issue. See original summary.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
StatusFileSize
new 3144610-2.patch8.16 KB
shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
Status: Active » Needs review

Hi @Vaibhav,

Created patch kindly check.

Thanks,
Shakti.

Status: Needs review » Needs work

The last submitted patch, 2: 3144610-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
StatusFileSize
new 3144610-5.patch4.19 KB

Testing to the Removed Test case.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 3144610-5.patch, failed testing. View results

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
StatusFileSize
new 3144610-8.patch5.25 KB

update patch #5 forgot to add AccessToken.php file for the added new title field. and working on install file for an existing installed module use case (need to create install file to Updating Database Schema/Data).

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
Status: Needs work » Needs review
shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
StatusFileSize
new 3144610-10.patch6.65 KB

Added install file to Update Database Schema/Data for an existing module, Kindly check.

sharma.amitt16’s picture

sharma.amitt16 commented
Status: Needs review » Needs work

@shakti thanks for the patch. Please find my small suggestions here

  1. +++ b/access_unpublished.install
@@ -0,0 +1,46 @@
+    ->installFieldStorageDefinition('title', 'access_token', 'access_token', $storage_definition);

    break the parameters in separate line to keep the line length to 80 characters. 

  2. +++ b/access_unpublished.install
@@ -0,0 +1,46 @@
+  $config_factory = \Drupal::configFactory();

    We can merge these two changes in single hook_update. 

  3. +++ b/src/Form/SettingsForm.php
@@ -43,6 +43,16 @@ class SettingsForm extends ConfigFormBase {
+      '#description' => $this->t('Default None of the tokens name.'),

    Description is little bit confusing.

    Would you please modify this to

    Default value of token name is 'None'.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
StatusFileSize
new 3144610-12.patch6.61 KB

Thanks, @sharma.amitt16,

I have updated above point, kindly check.

shaktik’s picture

shaktik
he/him
Primary language Hindi
Location New Delhi
commented
Status: Needs work » Needs review
generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

mably made their first commit to this issue’s fork.

mably opened merge request !35

mably’s picture

mably commented
Title: Add name to the token » Add a label to the access token
Version: 8.x-1.0-beta2 » 8.x-1.x-dev
mably’s picture

mably commented
Status: Needs work » Needs review
Issue tags: -Needs tests

MR created with some tests.

Wondering if we should add a config option to enable/disable this feature.

So people who don't need it won't be bothered with the extra fields/infos.

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented

Looking at this feature. I like what I see so far. The configured default of "None" is a little wierd to me but I can't come up with better suggestions other than leaving it un-named all together. That label is going to be subjective. I can see where people are going to want to add token support to that in the future... that's for another day.

The other thing is... the default is "None" but the upgrade does nothing to label all the tokens prior to the upgrade. They will effectively have NULL as that field value and I'm not sure if that won't bite someone in the butt later. Thoughts?

I really like where this is going though.

mably’s picture

mably commented

"None" was in the original patch, I would rather go with an empty string too.

I can change the hook to process all existing tokens.

mably’s picture

mably commented

Suggestions have been implemented.

generalredneck’s picture

generalredneck
him/his
Primary language English
Location Texas, USA 🇺🇸
commented
Status: Needs review » Fixed

This works excellently in my manual testing. It's going into 1.x-dev

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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