The content types selected at admin/config/people/flexiaccess are used to populate the autocomplete field, but any Node ID may be entered at user/{uid}/flexiaccess and it is not validated.

CommentFileSizeAuthor
#13 flexiaccess_drupal_html_id.patch4.3 KBrudolfbyker
#11 flexiaccess_validation_conditional_on_button.patch2.04 KBrudolfbyker
#6 flexiaccess-content_type_restriction-2984644-6.patch2.59 KBgisle
#4 Proposed_fix_for_issue_2984644.patch2.53 KBrudolfbyker
#2 flexiaccess_validation.patch2.38 KBrudolfbyker

Comments

fmr created an issue. See original summary.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented
Status: Active » Needs review
StatusFileSize
new flexiaccess_validation.patch2.38 KB

Here's a patch.

  • Move validation to the validation function.
  • Use form_set_error to inform the user what went wrong.
  • Replace entire form during AJAX event to allow form_set_error to work properly. This allows the form to even work without AJAX.
  • Add a validation function that checks the node type.
rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

By the way there is a comment that says:

It seems silly to re-query the titles every time. is there another way...?

It's not silly, since $form_state is saved in the database in any case. So just saving the $nid somewhere in $form_state and then using node_load() or some other query is perfectly acceptable.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented
StatusFileSize
new Proposed_fix_for_issue_2984644.patch2.53 KB

Silly me! I forgot two lines in the patch. Here's a new one.

gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs review » Needs work

Thanks for the patch - however it still needs some work. It gives the error message, but still adds the ACL for the node.

Here is how testing of a clean install of the patched module goes for me:

  1. Enable ACL for type "Article"
  2. Enter NID for a "Page". Press "Add node".
  3. Get error message: "Flexiaccess is not configured to manage nodes of type page."
  4. Enter NID for an "Article". Press "Add node".
  5. Result screen indicates that two ACLs has been created, both for the "Page" (wrong) and the "Article" (correct).
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs work » Needs review
StatusFileSize
new flexiaccess-content_type_restriction-2984644-6.patch2.59 KB

I think you just forgot to return from validation function before adding nid if there was an error.

  • gisle committed 3ea7e94 on 7.x-1.x authored by fmr 
    Issue #2984644 by fmr: Content type restriction is not respected
rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

Yes, you are right. I was working against the clock while making this patch, sorry.

I would add "return" after the first form_set_error, too, and move node_load to below the first if statement. It seems more consistent that way.

  • gisle committed 61baaf6 on 7.x-1.x authored by fmr 
    Issue #2984644 by fmr: Content type restriction is not respected tweaks
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented

fmr wrote,

I would add "return" after the first form_set_error, too, and move node_load to below the first if statement. It seems more consistent that way.

Of course. Pushed a new version where those tweaks are included.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented
StatusFileSize
new flexiaccess_validation_conditional_on_button.patch2.04 KB

There is still a problem when we try to remove the permissions from the users, since the same validation function runs when any button is clicked. Here I added a conditional to run validation only when the relevant button was clicked.

This patch is against the latest dev version, not against 7.x-1.2

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented

My patches were much worse than I thought... You can't add more than one node on the user page. I'll look into it soon.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented
StatusFileSize
new flexiaccess_drupal_html_id.patch4.3 KB

OK, I ran into this before. We should use drupal_html_id to generate a unique ID for the AJAX replace wrapper, and then store it in form_state. What happened was that every time the form is replaced, the ID changed. Now only the wrapper inside the form is replaced. The wrapper contains the table and the textfield, but not the buttons.

rudolfbyker’s picture

rudolfbyker
Primary language Afrikaans
Location South Africa
commented
Version: 7.x-1.2 » 7.x-1.x-dev

Patch at comment #13 seems to work fine when applied to the latest 7.x-1.x-dev version. I'll do some more testing with real users soon.

The 7.x-1.x-dev version is broken since commit https://cgit.drupalcode.org/flexiaccess/commit/?id=3ea7e94f49158247f7479... , which is my fault.

  • a89b09d1 committed on 7.x-1.x 
    Revert "Issue #2984644 by fmr: Content type restriction is not respected...

  • d01277f9 committed on 7.x-1.x 
    Revert "Issue #2984644 by fmr: Content type restriction is not respected...
gisle’s picture

gisle
he/him
Primary language Norwegian Bokmål
Location Norway
commented
Status: Needs review » Needs work

Reverted both flawed commits in this issue.

As a result of this patch in comment #13 no longer apply. It needs ta reroll and also a review by the community.