Helloooo :)

How have you been? :) You seem to be doing much better job than me in webform_views (no commits in the last month or so).

I've hit a bug today with webforms. It's rather easy to fix but difficult to decide which is the right fix for it, so I prefer to discuss it prior to doing any patches.

Here are the steps:

  1. Make sure your Drupal instance has 2 languages enabled. Let's make them English and French.
  2. Create a webform in any language (as far as I can see, the language does not matter here)
  3. Attach a "webform" field to a "page" content type.
  4. Enable translation for "page" content type and specifically enable translation of the "webform" field.
  5. Create a node in English and leave empty its "webform" field.
  6. Translate this node to French and fill in the "webform" field with our webform.
  7. Go to "view" this french translation and click on the submit webform link.
  8. Your browser will end up on this kind of page fr/form/[webform-id]?source_entity_type=node&source_entity_id=[nid].
  9. Bzzzz: at this point webform module already does not recognize that this webform (shall it be submitted now) must be associated with our page node.

The problem resides in the following snippet of ./webform/src/WebformRequest.php:

    // ---- We load the $source_entity in its default language. Which will be English per our steps to reproduce.
    $source_entity = $this->entityTypeManager->getStorage($source_entity_type)->load($source_entity_id);
    if (!$source_entity) {
      return NULL;
    }

    // Check source entity access.
    if (!$source_entity->access('view')) {
      return NULL;
    }

    // Check that the webform is referenced by the source entity.
    if (!$webform->getSetting('form_prepopulate_source_entity')) {
      // Get source entity's webform field.
      $webform_field_name = WebformEntityReferenceItem::getEntityWebformFieldName($source_entity);
      if (!$webform_field_name) {
        return NULL;
      }

      // Check that source entity's reference webform is the current YAML
      // webform.
      if ($source_entity->$webform_field_name->target_id != $webform->id()) {
        // ---- We fall into here because English node has no data in the "webform" field. Yet current language is French and it makes more sense to look up french (current lang) translation and check the field data for that translation.
        return NULL;
      }
    }

Do you follow me? I am not sure how big this bug is because it's rather unlikely that people will have different webforms for different languages (well, at least I think so, but god knows what actually happens out there in the wild). In my particular case I'll simply disable translation of my "webform" field in order not to hit this bug - luckily my business logic allows disabling translation of the "webform" field.

CommentFileSizeAuthor
#30 2861690-source-entity-multilingual-30.patch2.39 KBbucefal91
#24 2861690-source-entity-multilingual-24.patch32.93 KBbucefal91
#24 interdiff.txt1.3 KBbucefal91
#21 interdiff.txt11.81 KBbucefal91
#21 2861690-source-entity-multilingual-21.patch32.93 KBbucefal91
#19 2861690-source-entity-multilingual-19.patch29.49 KBbucefal91
#19 interdiff.txt10.2 KBbucefal91
#15 2861690-source-entity-multilingual-15.patch19.29 KBbucefal91
#6 2861690-source-entity-translation-5.patch2.12 KBbucefal91
#5 2861690-source-entity-translation-5.patch0 bytesbucefal91
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

bucefal91 created an issue. See original summary.

jrockowitz’s picture

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

@bucefal91 I am busy preparing for DrupalCon where I will most definitely be mentioning the Webform Views module in my presentation.

Even though, I am not doing much with translating webforms. I do follow the problem.

I think the WebformRequest handler should include the current request's language when looking at Webform node's Webform field.

I can see a slight edge case where a Webform node's translations might need to point to a different Webform or simply not display a webform.

I think the test is going to be harder to write than the actual fix.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Alright, I'll take the lead on the patch then.

Is it gonna be Baltimore? Shoot, I gotta put harder on webform views then so it shines in glory during the presentation :)

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Priority: Normal » Major

Changing status to 'Major' since this is definitely something that needs to be fixed.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
2861690-source-entity-translation-5.patch0 bytes

I am starting to advance on this ticket. My plain is the following:

  1. Add tests that touch this bug
  2. Fix the bug :)

Sounds like a hell of a plan :)

But cutting to the business, I want to extend your webform_test_translation module and enrich it with necessary dependencies + config/install files. Namely, I want to add "webform" entity reference to user entity and then enable its translation.

Then I'll write a test that will trigger a fail per the steps to reproduce in the main section of this ticket.

I've already added the necessary changes into the webform_test_translation module, but for some reason when I run webform phpunit tests locally, I am receiving tons of fails/errors. So I basically encounter myself without a fully operable local development environment. Therefore I think I'll just upload my patch here so I can see if all the existing tests pass after my modifications in webform_test_translation.

Is it OK for me to mess with the webform_test_translation module? or you prefer me to create a new test module and put all my stuff in there?

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
2861690-source-entity-translation-5.patch2.12 KB

mmm, for some reason the patch got uploaded with 0 bytes.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Hm... sorry to be such a clumsy guy, but I would expect the D.org infrastructure to queue my patch for testing and apparently it didn't. That was the point since I had issues executing the tests locally.

Do you know if I do something wrong or is there any "special"/magic button I forgot to click on? Of course, I can dedicate a couple of extra hours to convincing my local phpunit into running webform tests which I probably will do over the weekend if I can't get my patches tested on d.org.

jrockowitz’s picture

jrockowitz CreditAttribution: jrockowitz as a volunteer and at The Big Blue House commented
Status: Active » Needs review

@bucelfal91 This ticket's status just needs to be changed to the 'Needs Review' to trigger the testbot.

I will be the first to admit that I am only slowly adopting PHPUnit because I really like using SimpleTest's UI.

I do have some notes about running PHP Unit from CLI.
http://cgit.drupalcode.org/webform/tree/docs/RELEASE-NOTES.md

I also have found that running PHPUnit within PHPStorm works pretty well.

Below is the actual bash script I use to setup my local dev environment.

  # Setup unit tests.
  # @see https://www.drupal.org/node/2116263
  cd $DRUPAL_D8_WEBFORM_DIRECTORY/core
  [ -f phpunit.xml ] && rm phpunit.xml;
  cp phpunit.xml.dist phpunit.xml

  sed -i -- 's|<env name="SIMPLETEST_BASE_URL" value=""/>|<env name="SIMPLETEST_BASE_URL" value="http://localhost/wf"/>|g' phpunit.xml
  sed -i -- 's|<env name="SIMPLETEST_DB" value=""/>|<env name="SIMPLETEST_DB" value="mysql://drupal_d8_webform:XXXXXXXXXXX@localhost/drupal_d8_webform"/>|g' phpunit.xml
  sed -i -- 's|<env name="BROWSERTEST_OUTPUT_DIRECTORY" value=""/>|<env name="BROWSERTEST_OUTPUT_DIRECTORY" value="/var/www/sites/d8_webform/sites/default/files/simpletest"/>|g' phpunit.xml
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Yes, today in the morning when I got up I was like "Duh.. you didn't put the issue in "needs review" state :D" Thank you.

I haven't done an intensive work with phpunit in D8, although I do agree with you that SimpleTests UI was very good. I believe there must be some plugin or something that one can put on top of phpunit in order to get some pretty and more comprehensive output.

The absolutely cool feature of D8 testing framework is the support of JS via PhantomJS browser. A few months ago I did a presentation on software testing and D8 testing in particular and was amazed to see that now we can test even javascript behavior in automated tests. I mean, now there's just no excuse to release code with a bug :)

I am able to boot up phpunit and it executes the tests, but its failures make me think that there is some difference in my local environment and the environment where d.org does its tests.

I saw your email, so I think I better spend these 2 days on webform views so they shine in glory on the due day. I know you told me with a certain anticipation about the fact, but I've been kept busy with other things in my life and never could fully dedicate myself to webform views.

Stay tuned on this issue. I shall come back with a failing test and then will introduce a bugfix. But you basically give me a green light to expand a little bit your webform_test_translation so it sets up an environment where I can reproduce the bug? Or you want me to create a new test module for this purpose?

jrockowitz’s picture

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

@bucefal91 You have the green light to do whatever you need to fix/improve translations. Expanding webform_test_translation is fine.

It is very important to note that I think Drupal.org's testbot is having some issues. I am seeing random tests failing only on Drupal.org.

jrockowitz’s picture

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

@bucefal91 I see you are back in action. Is there any chance you can help with this issue?

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

hello! Ok, let me try to tackle it. I'll give it a shot in one of these days.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Jacob, let me share what I have so far:

  1. As far as I can see from looking at the code the problem is still there.
  2. The solution I would propose is to load the source entity and then test if it has a translation for the current language (and use that translation instead of original (untranslated) entity)
  3. This means we need to inject language manager into WebformRequest
  4. Because the language manager right now is not injected, I couldn't introduce any failing Unit test (I just cannot reproduce the conditions where the bug would manifest for lack of language context). Nonetheless I'd like to try to cover it with a unit test instead of functional.

With all that being said, I am going to inject the language manager; will write a failing test; will fix up WebformRequest so it passes the tests.

If my plans works then my patch #6 can be disregarded because I was trying to tackle it on the functional/kernel tests level.

By the way, my local environment now runs the phpunit tests successfully :)

jrockowitz’s picture

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

Besides trusting your judgment, it makes sense that the WebformRequest service would need the language manager.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
2861690-source-entity-multilingual-15.patch19.29 KB

Let's see if you're gonna like it. Since you said you trust my judgment, I took some initiative.

This method WebformRequest::getCurrentSourceEntity() seems to me like a good candidate for plugin type. Namely, it describes a specific self-contained, reusable action that can be implemented in multiple ways.

So I ventured to define a plugin type "SourceEntityDetect" and coded 2 implementations (I just copy-pasted the source code from ::getCurrentSourceEntity() into 2 plugins. One examines query string and detects source entity there (if possible); another one examines current route parameters and suggests from there. The plugins are weighted, that allows to prioritize them. I prioritized the QueryString.php since that snippet was running before the RouteParameters.php snippet in the original code.

The profits, as I see them:
More extensible system - if tomorrow you're gonna need yet another way of picking source entity, you'll only have to implement a plugin (a rather self-contained change which reduces probability of messing anything else up unintentionally). Other advanced developers/users of webforms have an extra chance to adapt your module towards their needs.
Instead of having a relatively thick service in WebformRequest.php we have now 2 (one very thin and another just slightly thinner).
Unit testing becomes dramatically easier. You get the chance to test each plugin separately.

I did code a Unit test that was covering ::getCurrentSourceEntity() but it didn't feel right. I will use the test cases I've come up with and will just adapt them for the QueryString.php plugin.

What do you say? Does it look attractive to you? Or you wouldn't like this change? Locally I have 100% pass. The patch I am attaching introduces no change in functionality, it only changes the architecture. If you agree to this architectural change, then I will enrich it with unit tests for both plugins and a fix of the bug itself.

Status: Needs review » Needs work

The last submitted patch, 15: 2861690-source-entity-multilingual-15.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

I would have never thought to add and maintain a new plugin. I am not sure I am ready to support it. At the same time, you are probably right.

Alexander, are you ready to become a co-maintainer of the Webform module? My only expectation would be that you appear every month or so and throw out a few awesome patches? No is a fine answer and Yes would be awesome.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

hello!

I do not think there is a high maintenance cost associated with this new plugin type. It's more of one-time fixed cost to introduce it and then rarely anyone would return to it. But that's your call. :)

Thank you for crediting me! I am glad to know my humble and limited involvement is so well-seen by you. I would accept your kind offer of co maintainership. Throwing a few patches a month is not such a high price, after all :) The only concern I have is whether I will not be disturbing you? Like with this initiative. You definitely have some ideas, visions and expectations about future and code architecture; my vision might differ fore we are 2 distinct humans. I will seek to adapt and not create too much of disruption.

I guess let's do it. I will happily work on webforms. But if at any point you feel like it's working poorer than when you were in the "solo" mode - let me know and I can return my current role of situationally supplying a patch here and there.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review
FileSize
interdiff.txt10.2 KB
2861690-source-entity-multilingual-19.patch29.49 KB

.. in the meantime, as expected, the tests themselves now are easy catch. I add unit tests for both QueryString and RouteParamters plugins. The bug is still not fixed. At the moment I've only covered the existing functionality.

So the interdiff contains only the tests. I will have a look into this 1 fail. My localhost still shines 100% pass.

Now yes, I will inject the language manager into QueryString and will make it fail a unit test; then I'll update QueryString to catch up on the multi language.

Status: Needs review » Needs work

The last submitted patch, 19: 2861690-source-entity-multilingual-19.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
FileSize
2861690-source-entity-multilingual-21.patch32.93 KB
interdiff.txt11.81 KB

This is the patch that:

  • Introduces the test case which fails on original code (basically, this new test case exposes the very bug we are trying to address in this issue)
  • Fixes the bug in QueryString.php by checking if the looked up $source_entity has a translation for the current language (and if it has, then using the translation instead of the original one).
  • Fixes the other fail that drupal testbot was pinpointing.

The reason I was having 100% pass on my localhost all the time during development of this issue is because I was running only phpunit based tests, i.e. Unit, Kernel, Funcitonal and was not running simpletest-based tests.

As far as I can judge this patch is commit ready :)

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: 2861690-source-entity-multilingual-21.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
interdiff.txt1.3 KB
2861690-source-entity-multilingual-24.patch32.93 KB

Now it should work. I overlooked one last thing.

jrockowitz’s picture

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

@bucefal91 It is good to have another set of eyeballs looking over the code and I need to reduce the webform module's bus factor.

I also want you to get recognition for your contributions to Drupal and being a co-maintainer of the Webform module is an excellent way to be recognized. I am going to create a ticket to get you commit access so that your name will appear under 'Maintainers for Webform' with each commit. I will also update the webform's composer.json file. If you have time, please join the #webform and #webform-dev Drupal Slack channels.

I just got a little slammed with some paid work and I need to focus on tagging RC2 this week. I will review this ticket next week and we can get it committed to RC3.

jrockowitz’s picture

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

@bucefal91 The code as always is excellent.

My main concern/issue is we are adding a new plugin to solve a bug while trying to work towards a stable release. I am completely open to implementing new features and API changes, but they should to handled via a dedicated ticket with a change record.

BTW, I am also completely guilty of adding API changes to bug fixes and feature requests. Adding a new plugin is a major API change.

I suspect your new plugin might also be able to fix/address #2919125: Not picking up name for "Submission URI" for Commerce 2.x pages.

Would you be open to isolating the immediate bug fix for this ticket and create a new ticket with a change record draft for the new plugin?

Finally, thanks again for all your help and awesome code.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented

Yes, Jacob, I understand you. I haven't been using the change records on d.org before so I am not strict about it :) But I'll keep an eye for it.

And yes, that ticket about Commerce "submitted to" source entity should become a much easier thing when this plugin type lands. That was exactly my point in introducing it - so we can place all such tweaks and IF`s about how to fetch a source entity into dedicated plugins (keeping it in plugins should help to maintain it more organized rather than keeping all in some kind of plain ::getSourceEntity() method).

So, to confirm I get your words in the correct way: I should file a feature request that suggests to implement this new plugin type. Then I should make this ticket and the #2919125: Not picking up name for "Submission URI" for Commerce 2.x pages depend on this feature request. And then I should toss corresponding patches into each ticket? Is this what you meant? :)

jrockowitz’s picture

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

@bucefal91 #28... Yes, let's create a dedicated ticket for the new plugin and then fix this ticket and #2919125.

I am targetting the next release for March 1 so you should decide if want to get this plugin merged ASAP or after the next release. I am fine with either option.

Maintaining 'Change records' is going to make it easier to tell people about major API changes and improvements. I really wish that contrib modules supported semantic versions.

bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
Related issues: +#2944203: Improve maintability of source entity detection
bucefal91’s picture

bucefal91 CreditAttribution: bucefal91 at Websolutions Agency commented
FileSize
2861690-source-entity-multilingual-30.patch2.39 KB

Now I can supply here the minor tweak that fixes the bug and covers it with a unit test in QueryStringWebformSourceEntity.

jrockowitz’s picture

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

  • bucefal91 committed 3a4178a on 8.x-5.x 
    Issue #2861690 by jrockowitz, bucefal91: Fixing the source entity...
bucefal91’s picture

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

Committed.

Status: Fixed » Closed (fixed)

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