Based on https://www.drupal.org/project/webform/issues/2861690#comment-12478835

We should introduce a new plugin type whose goal would be to suggest a source entity type from the current context. This should help with extending webform module by 3rd party developers and offload the pressure off WebformRequest::getCurrentSourceEntity() so it doesn't grow infinitely as we introduce more and more logic into how to detect a source entity.

The patch is coming shortly.

CommentFileSizeAuthor
#15 2944203-source-entity-plugin-type-15.patch32.56 KBbucefal91
#15 interdiff-14-15.txt1.4 KBbucefal91
#13 2944203-source-entity-plugin-type-13.patch32.52 KBbucefal91
#13 interdiff-10-13.txt1.83 KBbucefal91
#10 interdiff-4-10.txt19.64 KBbucefal91
#10 2944203-source-entity-plugin-type-10.patch32.52 KBbucefal91
#7 2944203-source-entity-plugin-type-7.patch32.46 KBbucefal91
#4 2944203-source-entity-plugin-type-4.patch33.07 KBbucefal91
#2 2944203-source-entity-plugin-type-2.patch33.8 KBbucefal91
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bucefal91 created an issue. See original summary.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Active » Needs review
FileSize
2944203-source-entity-plugin-type-2.patch33.8 KB

This is the patch. It contains the refactoring from https://www.drupal.org/project/webform/issues/2861690#comment-12452024 but not the bugfix. (I deliberately also omit a test case that exposes the bug so we do not break test pass if we commit this issue).

..working on the change record now.

Status: Needs review » Needs work

The last submitted patch, 2: 2944203-source-entity-plugin-type-2.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review
FileSize
2944203-source-entity-plugin-type-4.patch33.07 KB

https://www.drupal.org/node/2944230 - this is the change record.

The new patch should pass the tests. + I fixed some coding styling.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Assigned: Unassigned » bucefal91

This is a huge API improvement.

Please are some general comments/questions...

  • Should we remove the 'Detect' and just call these plugins WebformSourceEntity plugins?

If you are okay making the above and below changes and updating the change record, I will do a quick review and mark this RTBC.

BTW, I am reassigning the ticket to you so that you can visible credit is the Webform issue queue.

  1. +++ b/src/Plugin/WebformInternal/SourceEntityDetect/QueryString.php
@@ -0,0 +1,164 @@
+namespace Drupal\webform\Plugin\WebformInternal\SourceEntityDetect;

    Can you please remove the WebformInteral namespace? This is not a core pattern and not really needed.

    If we need to mark something as internal we can add @internal to all the related classes.

    We should call this class RouteParametersWebformSourceEntity to match the naming conventions used by other Webform plugin name (except for WebformElements). 

  2. +++ b/src/Plugin/WebformInternal/SourceEntityDetect/RouteParameters.php
@@ -0,0 +1,81 @@
+namespace Drupal\webform\Plugin\WebformInternal\SourceEntityDetect;

    Remove /WebformInterna'/ and change to QueryStringWebformSourceEntity

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs review » Needs work
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review
FileSize
2944203-source-entity-plugin-type-7.patch32.46 KB

I've gone through all your feedback. Will update now the change record.

Status: Needs review » Needs work

The last submitted patch, 7: 2944203-source-entity-plugin-type-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Change record updated. :)

Thanks for willing to commit this :) I wasn't exactly sure whether you will like it because it does introduce some disturbance in the development and doesn't reward with any immediate end-user feature.

Let's see what the tests say. They will show if I overlooked anything while renaming/moving definitions.

---------
edit: actually they did :)

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review
FileSize
2944203-source-entity-plugin-type-10.patch32.52 KB
interdiff-4-10.txt19.64 KB

Let's see this one.

The last submitted patch, 7: 2944203-source-entity-plugin-type-7.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented

The code makes perfect sense because it splits the querystring and route detection into dedicated plugins definition with isolated business logic. This refactoring allows for an inevitable third source entity detection method to be easily added and tested.

  • The first time you do something, you just do it.
  • The second time you do something similar, you wince at the duplication, but you do the duplicate thing anyway.
  • The third time you do something similar, you refactor.

-- http://wiki.c2.com/?ThreeStrikesAndYouRefactor

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
interdiff-10-13.txt1.83 KB
2944203-source-entity-plugin-type-13.patch32.52 KB

A little more of coding standards.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

As far as I can judge, this is ready, Jacob.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
interdiff-14-15.txt1.4 KB
2944203-source-entity-plugin-type-15.patch32.56 KB

Just a small improvement in the @see comments.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Needs review » Reviewed & tested by the community

I am marking this RTBC so that have can feel the 'reward of the merge', right after all the tests pass.

  • bucefal91 committed 6f6cedd on 8.x-5.x 
    Issue #2944203 by jrockowitz, bucefal91: Exposing source entity...
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Reviewed & tested by the community » Fixed

Yupii! Committed. Should I mark the change record as published or that should happen when a new release is tagged?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.