Convert Quiz to D7 Entity API

Also see #2314145: Convert Questions to D7 Entity API

This patch is huge, I don't even know where to start (and neither do the testbots).

Please break it up into smaller issues, it appears like this is a complete fork of the module with no upgraded tests or migration path.

I see that a lot of the changes are just renaming files and it is cluttering up the patch file. Those changes were already proposed in other issues here (#2351119: Split hook implementation from main module file, #2351113: Use PSR-0 test cases). Can you isolate the major changes so we can do more targeting analysis/testing/migration?

Thanks, the patch does actually apply fine. I did it manually.

There are less changes in here than it seems, there's just a lot of noise with the files being renamed.

But there's a lot of failures:

Results
879 passes, 236 fails, 18 exceptions, and 1618 debug messages

There are also a lot of typos, errors in UI/documentation but I can clean those up for you.

A few questions, just based mainly on the code b/c the module has a lot of fatal errors while testing it.

1) The quiz entity type stuff is great, but how do you create a Quiz? I did not see anything in the UI. EDIT: I found it, in admin/content/quiz. But I wasn't able to add a Quiz without fatal errors. My next question would be how do we get the quiz to show up in content, but we could probably do that with entityreference.

2) Why is everything called staticCallback() ? It gives no indication of what the method does, and there's no reason to explicitly name a function "staticXX". Just call it what it is, getStatsForm() or getRenderedQuestion() or whatever. It has already been declared static.

3) Big question: Is it really necessary to convert our already working (and D7 best practices) Form API to an OO structure? To me it seems like there is no advantage. You still have to have a workaround to call them statically in Drupal 7 so what is the point? There is no benefit to DI or lazy loading them. When you hook_form_alter the form it's just going to plain old form API anyways. Except it's going to be harder for the D7 community because now the form ID is called "Drupal\quiz\Form\QuizAnsweringForm::staticCallback": Doesn't this seem like a hacky way to get this into Quiz?

I'm not sure what error the "@" is masking, but we shouldn't have to hide errors to get it to work. It's probably masking the error that happens when it tries to call hook_form_Drupal\quiz\Form\QuizAnsweringForm::staticCallback_alter().

    $form_id = 'Drupal\quiz\Form\QuizAnsweringForm::staticCallback';
    $question_form = @drupal_get_form($form_id, $this->quiz, $this->question, $this->page_number, $this->result);

4) I do not see a migration for Quiz node content, it only copies the base node record. Many people have fields on the Quiz, those won't work any more?

5) This breaks a lot of existing Views and SQL queries because of the table and field naming changes. A lot of the quiz community depends on these not changing. Is it important enough to rename the tables and primary keys? We want to be as compatible as possible, so we kept the names of tables and fields the same, so that people coming from 7.x-4.x don't have a tough migration.

6) Many of the existing hook_update functions are changed. These updates have already been deployed to 100s+ of sites, so they will not run. We would need to change them to run after the ones currently in quiz.install.

7) Do any of the other OO changes have any benefit other than just being OO? I am with you on the Quiz entity types, and I love OO design but the non-entity OO work seems like it's work just for the sake of being OO. Some of this I feel needs to wait until D8, where it is accepted (and required), it actually might hinder D7 stable progress and contribution, since it breaks a lot of standard D7 hooks and alters and not a lot of other modules do these things.

(2) Are you just confuse about naming? I use static method as shortcut to create object, it's like simple function, but I do not want create redundant functions, so I have static methods.

Okay, but that's very non-standard. It's also still procedural programming, you are just using a static callback. I see no need for this. At the very least your static methods should be called what they actually do. It would be like calling your functions "function()"

(3) This patch is not yet OOP, I am trying moving things to classes, it makes it easier for people to understand the full quiz code. I will try remove the hacking around @drupal_get_form(…).

I don't think you will get around this, no module does this. Plus, you broke the ability for a Drupal 7 developer to use hook_form_FORM_ID_alter(). (https://api.drupal.org/api/drupal/modules!system!system.api.php/function...)

I see no reason to convert Quiz's (or ANY module's) forms to a class when Drupal 7's form API is not class based. We get no benefit other than alienating the D7 community. I think it's *more* confusing to understand the Quiz code since we still have to use procedural programming for Drupal 7's form API.

(5) I agree, but keep wrong meaning table name/columns just for compatible, I think it's not the reason. I can follow up with fixes if you can find anything with views integration.

Anyone with custom queries/views will have to remake them as the base tables changed. 4.x users have repeatedly requested an upgrade path. I see no benefit here. We should change them in D8 when people have to change things anyway.

(7) I have no idea, Drupal 8 breaks everything, even its long time procedure coding style. I think this is also time for quiz to make same change.

Drupal 7 is still mostly procedural! And so are the modules. It just seems like we are moving to OO for no reason. There is no inheritance going on, it's just converting procedural code to OO code. From a design perspective it's still procedural, just wrapped in OO. It's going to be a tough upgrade for the thousands of 4.x users waiting for a stable 5.x release.

I would like to move forward with testing/committing any Quiz/Question as an entity work if you can separate that out from the other parts of the module that changed. I can also help with that.

Wow...
There is so much here. I can see you've done quite a lot of work on this @thehong, but this patch is unnecessarily huge. Just reading the patch alone takes a significant time investment. Contributing to that problem, whitespace changes and rewording are out of scope for this work. Arguably the renaming is too. The patch should really only include code relevant to the entity conversion. Maybe you could create a separate patch for the cleanup work?

The form conversion bits of this are not only confusing, but also out-of-scope for the entity conversion. It makes no sense to remove working form_api code. TBH I'm not sure how/if that code even works without breaking the form_api entirely. Likely that's part of why tests do not pass. Also, creating a class and calling it statically is not improving the architecture of this code. Its just obfuscated procedural programming that's further obfuscated by the naming of your callbacks. staticCallback() is not a descriptive method name at all. I should be able to tell what it does just by reading the name.

Really, I think @djdevin's suggestion to just separate out the things relevant to the entity conversion is a good one. This is just too much to add in one patch.

Did you even read what we said? You didn't implement any of the suggested changes! I won't be committing this patch as it is, ever. I appreciate your efforts but now you are just flat out ignoring a good patch review.

- You still have usages of staticCallback() everywhere
- There's a new class for "HookEntityInfo::HookImplementation" - no Drupal 7 developer would appreciate this, because this is absolutely the wrong way. You moved hooks into OO for absolutely no reason other than obfuscation. Please look at other well-known Drupal modules and see that nobody does this as it's bad practice.
- Drupal coding standards are violated across the board (see https://www.drupal.org/coding-standards)
- You still have a dependency on xautoload that I requested you not to use
- You still renamed all the tables and fields which will break custom integrations and views. We're already at alpha4 which really means we should not be making huge changes like this.
- As I said before there is no benefit to obfuscating non-entity forms that do not need or benefit from inheritance.
- Your patch is huge and cannot be reviewed (see http://webchick.net/please-stop-eating-baby-kittens)

The DX here is terrible, we don't want it moving forward. This module would be so removed from Drupal that no future maintainer would want to take it over.

I tested your patch very briefly (it's 2.53MB, there's no way I can review it all) and there are still serious issues with the module and upgrade path which will break all existing 4.x and 5.x users (4000+ installs) that aren't yet covered by automated tests. This is why we have a manual patch review process. I would be clinically insane to blindly commit a 2.53MB patch without rounds and rounds of review and testing. Since it's a such a huge patch it's going to be very difficult, almost unfeasible, for that to happen.

There is so much going on in this patch that we do not need, and gives no benefit. If you want to make change here, stick to the scope, break up your patch into smaller patches, and resubmit.

This issue should be about converting the existing "quiz" content type into an entity, using Entity API with best practices. If you can't (or don't want to) do that please let someone else work on it.

Cleaning issue since you forked.

See #2378365: Convert Quiz to D7 Entity API