Skip to content
Snippets Groups Projects

Draft: Module review

Closed Draft: Module review
issue/lms_certificate-3502628:3502628-general-review into 1.0.x
3 unresolved threads
Closed Marcin Grabias requested to merge issue/lms_certificate-3502628:3502628-general-review into 1.0.x
3 unresolved threads

Closes #3502628

Merge request reports

Merged results pipeline #407617 failed

Stage: build
Stage: validate

Merged results pipeline failed for 6bfcefa8

Closed by Marcin GrabiasMarcin Grabias 1 month ago (Jan 28, 2025 10:46am UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
modules/lms_certificate_fillpdf/src/Controller/CertificateController.php
1 1 <?php
2 2
3 // This should probably reside in the parent module.
  • C
    catch @catch ·
    Maintainer

    This isn't possible because it would introduce a dependency on drupal/fillpdf - which the parent module can't have.

    We can open a follow-up issue to convert this controller logic into a plugin, and then move the controller to the parent module and look for a plugin etc. but that's part of generically supporting multiple different certificate types on one site which isn't necessary for the absolute minimum.

    Edited by catch
  • Marcin Grabias
    Marcin Grabias @Graber ·
    Author Maintainer

    We could use a plugin system and generate that link from a plugin but ok, I agree that can be done later when we'll have at least one more certificate type.

  • Please register or sign in to reply
  • catch @catch started a thread on the diff
    • src/Hook/LmsCertificateHooks.php
    26 26 ->setLabel(new TranslatableMarkup('Certificate'))
    27 27 ->setName('membership_group')
    28 28 ->setDescription(new TranslatableMarkup('Reference to an entity providing a certificate'))
    29 // I think the below 2 settings are not needed, it's a dynamic refeerence and will never reference a group..
  • catch @catch started a thread on the diff
    • composer.json
    12 12 "require": {
    13 13 "drupal/lms": "^1.0",
    14 14 "drupal/dynamic_entity_reference": "^4.0@alpha",
    15 /* Since this is required by the child module only, maybe we should add a hook_requirements there instead? */
    • C
      catch @catch ·
      Maintainer

      I think we could consider splitting the entire fillpdf module out to its own project, but probably only once we support other certificate types, which could be a while.

    • Please register or sign in to reply
  • Marcin Grabias closed

    closed

    • Please register or sign in to reply