fillpdf_file_context table gathers too much context

@rprager thanks for the bug report. I think the real issue here is that FillPDF is storing so much information when it could just load the submission from the database. The reason this table exists is for the use of private files. It actually does load it anyway when checking access to them.

So what I'm thinking is that I can pare down the information stored and store just enough for the access-checking code to work, which is pretty much Node ID and Submission ID. I don't think anything else is used for access checking on the Webform-side anyway, and I'm using its access functions.

That said, if you have time to provide a patch to update the column type, I will commit that in the interim. TEXT vs. LONGTEXT isn't going to be a monumental change for most sites anyway.

I came up with a whole new way to do this in Drupal 8 that I'm going to backport to Drupal 7 for this issue, which is simply saving a normalized version of the FillPDF Link and then re-parsing that to determine the context on regeneration.

Liam, can you take a look at this patch? I don't have time right now to test it to my satisfaction (Webforms, UC Orders, UC Order Products), but I think it should work OK. I did some access testing with a regular user.

That is a big patch. Can you give an overview of what is changing? That will make it easier for me to review.

One thing I notice:

-  if (is_array($nids)) {
+  if (is_array($nids) && count($nids)) {
     $nids_uri = '&nids[]=' . implode('&nids[]=', $nids);
   }
   elseif (isset($nids)) {
     $nids_uri = "&nids[]={$nids}";
   }

If $nids is an empty array, it will run the elseif and you'll get "&nids[]=Array". I don't think that is what is wanted. How about casting to array first and forget about the elseif?

The doxygen comments aren't right. For example:

* @param int $fcid The fcid of the context object to load.

Should be:

 * @param int $fcid
     The fcid of the context object to load.

@Liam Morland Yeah. Primarily, it is refactoring so that I can turn FillPDF context (generation info) into a link and the other way around.

I use these when generating private files later and when checking access.

Finally, the install hook update converts things in the old format to the new format.

Could I bug you to apply the style fixes, Sir Drupal 7 Maintainer? :)

I really wanted to do some broad-strokes tests, e.g. can a PDF be generated, does access work properly, etc. Obviously we would need a fake remote service to simulate generating the PDF (maybe it just returns the same file all the time)...because this is sort of a big refactor. It'd be nice to do those before releasing a new stable, but I don't want to hold it up forever ¯\_(ツ)_/¯

@wizonesolutions If a patch has a lot of refactoring and some functionality change, I prefer to have two patches: One that just refactors with no functionality change and one that changes functionality. That way, it is easy to see the functionality changes and to verify that the refactoring doesn't have any side-effects. I haven't yet read the patch in detail. Is it feasible to separate out the refactoring from the functionality?

I just fixed a bunch of coding standards in #2612468: Coding standards and documentation, so this will need a re-roll.

I was referring to the style issues introduced by the patch. Every patch should at least not make things worse.

It would be good to get tests in, but it is a bunch of work. Perhaps a starting place would be things that have broken in the past during updates.

@Liam Morland: I did this since I was working on FillPDF at DrupalCon, but in general I will mostly leave the backport decisions/work to you :)

These refactors were necessary for the functionality. I could not have implemented the functionality without duplicating code. I avoided random style fixes that didn't have to do with the work in question.

I agree on tests. We could leave this in dev for a bit and see if anyone reports bugs.

Finally need this patch for a project. If it works, I'm going to commit it.

Is it feasible to do the refactoring in a separate patch from the functionality change?

Re-rolled.

@Liam Morland: No, not really. This approach is a backport from Drupal 8, so conceptually I had to make some things more similar to Drupal 8 in order to do it the same way.

I understand.

I got the same CI error in another project, so the test failure is not caused by the patch.

I'm going to see how client testing goes (I told them to re-test FillPDF-related functionality in general) and will probably commit it to dev and let it simmer for a bit. I want to do another stable soon. I'd kind of like to have reasonable assurance that this won't break things for people, though, since they won't be able to roll back their data easily (well, they'll have to use a backup) since the update hook mutates data in the fillpdf_file_context table.

Yes, it would be good to do another stable release. We can help with testing as well, though we only use it with Webform, so we're only testing that use case.

When we're ready, we should tag an -rc (or maybe a -beta) first and wait until it is a few weeks old before making a final release in the hopes that more people will test before the final.

Oh, great idea. Can we do rc releases alongside the existing stable?

Yes. See the drupal_reset module for an example of this. I make full releases when I have an -rc that is at least two weeks old with no release-blocker issues. I have made exceptions to this only when a security issue or critical bug is discovered.

Let's commit this and do that. The client using this patch has not reported any problems. +1

Can you re-roll this? It doesn't apply.

Please give this re-roll a final check and RTBC.

Looks good.

@Liam Morland: Don't commit this quite yet. It's got a bug. I will submit a fixed patch. The bug is that we don't actually execute the update in the update hook, so the context doesn't get fixed on old records. I just have to test this.

...that is not even the only bug. The link generation (at least in my version) doesn't seem to work at all. The structure is more or less correct, but the actual values are missing. Example:

https://example.com/fillpdf?fid=1234&nids[]=&webforms[0][nid]=&webforms[1][nid]=&webforms[2][nid]=&webforms[3][nid]=

This is for new records too. I can't fix those, but hopefully nobody has applied this patch, or if they have they know how to fix their own data and how they are generating PDFs.

OK. Bugs should be fixed. The primary problems were:

- NIDs being handled poorly. Even if it's a string, we shouldn't blindly make it an array without checking for non-empty.
- Webforms weren't being resolved to proper nid/sid arguments from the context.
- The update wasn't executing the query.

These are all resolved, AND I added a simple test for fillpdf_pdf_link(); Not the best as an integration test (it's really sort of a unit or kernel test that needs some mocks), but much better than nothing.

Tests pass locally.

I probably want to let this sit for a week or so (so anything that's going to break can break in production :D) and/or get some more community feedback. Let's leave it out of the RC or release, @Liam Morland.

Thanks. I was going to wait until the fixed issues auto-close before making the -rc. Once the release is stable, we can commit this and set it sit in dev for a while.

Yeah. If people test it without issue, great, but if it's just us committing it I'd like to minimize risk.

Found another bug. On Webforms, only the final sid is properly being included in the link for some reason. Here is a patch for that. The fix was obvious, and I have no idea how I missed this.

Thanks. I love the tests. I'll do a full review, probably on Friday.

This has been working well in production since last I commented. I think the bugs are finally ironed out, and it could get committed to dev.

Thanks. I will commit this to a feature branch 7.x-1.x-2600002-context now. People can test it if they want.

I have just released 7.x-1.11. In a couple of weeks, if this release does not need any fixes, I will merge the feature.