Skip to content

Rob Review GDPR Consent#10

Open
yanniboi wants to merge 47 commits into
7.x-1.xfrom
feature/d7/gdpr_consent
Open

Rob Review GDPR Consent#10
yanniboi wants to merge 47 commits into
7.x-1.xfrom
feature/d7/gdpr_consent

Conversation

@yanniboi

Copy link
Copy Markdown
Contributor

No description provided.

zserno and others added 30 commits April 10, 2018 16:21
This is now in line with the D8 version of this module.
The long description is now hidden with javascript.
These are notes for staff to put their rationale for why they have done this for auditors.
By switching to entity_ui_get_form page callback.
Comment thread gdpr_consent/gdpr_consent.install Outdated
'unsigned' => TRUE,
'not null' => FALSE,
'default' => NULL,
'description' => "The {users}.uid of the associated user.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What user is this? How is the user associated?

Comment thread gdpr_consent/gdpr_consent.install Outdated
),
),
'primary key' => array('id'),
'unique key' => array('name'),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be 'unique keys'.
You can also put revision_id in as a unique key.

'primary key' => array('id'),
'unique key' => array('name'),
'indexes' => array(
'name' => array('name'),

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't need to be declared separately as it's already declared unique.

Comment thread gdpr_consent/gdpr_consent.install Outdated
'unsigned' => TRUE,
'not null' => FALSE,
'default' => NULL,
'description' => 'The ID of the attached entity.',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Attached Entity"?

Comment thread gdpr_consent/gdpr_consent.install Outdated
'unsigned' => TRUE,
'not null' => FALSE,
'default' => NULL,
'description' => "The {users}.uid of the associated user.",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"Associated user"? Is this a revision-ed version of the field on the entity base table? Or supposed to be the creator of this revision? Or both?


foreach ($items as $delta => $item) {
if (!empty($item['target_id'])) {
if (!$valid) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand what this is doing? $valid is always TRUE in this function.

Comment thread gdpr_consent/gdpr_consent.module Outdated
}

foreach ($fields as $field) {
if (!empty($values = $form_state['values'][$field])) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't supported by all the versions of PHP that drupal supposedly supports. empty can only have variables in it.

  if ($values = $from_state['values'][$field]) {

Would be better.

/**
* {@inheritdoc}
*/
public function identifier() {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems superfluous. Why is this being added?

* Provides a user consent field type.
*/
function gdpr_consent_field_info() {
return array(

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add a property info callback so that wrappers can be used to access consent information

Comment thread gdpr_consent/gdpr_consent.module Outdated

$build['long_description'] = array(
'#type' => 'markup',
'#markup' => $entity->long_description,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need some sanitization

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants