πŸ‡ΊπŸ‡ΈUnited States @benjifisher

Boston area
Account created on 30 December 2009, about 14 years ago
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

Please try the beta3 version.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

Cleaning up during cron seems like a good idea. Thanks for suggesting it.

I think that #5 is a good point. Let's keep the highest current value in the table. One advantage of this change is that the site owner can use a simple database query to figure out what the next serial will be, and recognize whether the entity with the highest serial has been deleted.

I would like to get some test coverage for this change. At least test add some entities, delete some entities, run cron, add and delete som more, and make sure that we get the expected serial values at every step. If there is a way to reproduce the deadlock in an automated test, that would be great.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The 8.x-1.x branch of this project is no longer active, so I am moving this issue to the 2.0.x branch.

I will create a MR from the patch in #2 and see whether the tests pass.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I cannot reproduce this problem. Here are the steps I tried:

  1. Install Drupal 10.2 with the Standard profile.
  2. Enable the serial module.
  3. Add a Serial field to the Article content type.
  4. Add 3 Article nodes.
  5. Delete the third node.
  6. Add another Article node.
  7. Delete all nodes.
  8. Add another Article node.
  9. Check the database: SELECT * FROM node__field_serial;

The result is that the serial value was set to 5.

If you are still having this problem, please give similarly detailed steps to describe what you see.

πŸ“Œ | Serial Field | Help Text
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I updated the patch for the 2.0.x branch and created a MR from it. Tests are passing.

Thanks to all who contributed to this issue. The new text is a big improvement!

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The 8.x-1.x branch of this project is no longer maintained, so I am moving this issue to the 2.0.x branch.

I want to think about this issue for a bit, but I will at least create a MR from the patch in #2 so that we can run automated tests on it.

πŸ“Œ | Serial Field | Help Text
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The 8.x-1.x branch of this project is no longer active, but I think the patch will apply, with a minor change, to the 2.0.x branch.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

Now that I have enabled GitLab CI testing for this project ( πŸ“Œ Enable testing with GitLab CI Fixed ) the tests showed a couple of problems, so I added some small commits to fix them.

Thanks to everyone who commented on this issue. The original report, the clarification of the issue, the "works for me" comments, and of course the fix in the MR all contributed to getting this issue done.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I agree with Comment #3: based on the Drush error message, this issue seems to be a duplicate of πŸ› Error message: There was an unexpected problem serving your request RTBC .

I just accepted the MR for that issue, and I will create a new release. Please re-test and see whether the problem has been fixed.

I am setting the status to PMNMI. If the re-test shows there is still a problem, please update the status. If there is no further response for two months, I may close this issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@pwolanin:

Thanks for taking a look. I will merge this now, then disable the legacy DrupalCI testing, and then move on the other RTBC issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

+1 for all the reasons above.

Kristen is thoughtful and cooperative, both of which are important for this role.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The other option is to just remove the hook and document that users have to enable the other submodules manually.

That is what I was thinking.

Even when the submodule is enabled automatically, there is still another step (I think): the user has to add a REST export field to the view before the field shows up in the export, right?

Also, removing the hook would be following the principle of least surprise. We do not expect modules to be enabled without being selected. Are the submodules even listed in the confirmation page if you enable modules in the admin UI?

Maybe some sites enable those modules but do not need to export those fields. If we force them to enable the submodule, then we are adding unneeded code to their sites.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I wonder how this issue will be affected by πŸ› Serial not generating on entity save in certain contexts Needs work . I am adding that as a related issue.

Looking at the patch in #23, I have a few comments.

1.
+++ b/src/SerialSQLStorage.php
@@ -73,7 +73,7 @@ class SerialSQLStorage implements ContainerInjectionInterface, SerialStorageInte
   /**
    * {@inheritdoc}
    */
-  public function generateValueFromName($storageName, $delete = TRUE) {
+  public function generateValueFromName($storageName, $externalValue = NULL, $delete = TRUE) {

Normally, when adding a new optional parameter, we add it after the existing parameters, for backwards compatibility (BC).

2. @@ -81,9 +81,17 @@ class SerialSQLStorage implements ContainerInjectionInterface, SerialStorageInte
...
+      if (isset($externalValue)) {
+        $sid = $connection->insert($storageName)
+          ->fields(['sid' => $externalValue, 'uniqid' => $uniqid])
+          ->execute();
+      }
+      else {
+        $sid = $connection->insert($storageName)
+          ->fields(['uniqid' => $uniqid])
+          ->execute();
+      }

This can be simplified. Something like this:

$values = [...];
if (...) {
  $values['sid'] = ...;
}
$sid = $connection->insert(...)
  ->fields($values)
  ->execute();
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@madelyncruz:

Thanks for reporting this issue, and working to fix it.

The usual practice is to assign an issue to yourself when you are working on it, then unassign it when it is ready for review.

I made some suggestions on the MR, but there are some bigger points as well.

First, I think that hook_entity_insert() is the wrong hook to use here. It means an extra save, which might even lead to an infinite loop. We should use hook_entity_create() or hook_entity_presave() instead, both of which are called before the entity is saved. See Entity CRUD, editing, and view hooks.

I suggest using hook_entity_create(). This will be called on new entities, not updated ones. That should be enough: it saves the trouble of checking whether the field is already populated and quitting if it is. Also, there might be some edge cases where the site uses custom code to remove the serial field in special cases. In that case, we should not add it back whenever the entity is updated.

The second point is that a change like this needs to have an automated test to go along with it. The test should do at least the following:

  1. Prove that there is really a problem, and that the proposed changes fix that problem. The test should fail without the change and pass with the change.
  2. Confirm that there is not an off-by-one error. Especially since the code comes with a comment about subtracting one, we have to be careful about that.
  3. Guard against future changes to the codebase. Maybe some day we will think of a simpler way to manage the serial field so that you do not have to subtract one. If that happens, then we have to make sure that this code still works, or the test fails so that we know to update it.

The second and third items can be covered by the same test.

If you are not familiar with writing PHPUnit tests, then at least come up with some lines of code to show the steps. I often use drush php for that sort of thing. Then I can convert it into a test.

The last point is that we may be able to simplify other code in this module once we implement the right hook. (Maybe we can remove code in Drupal\serial\Plugin\Field\FieldType\SerialItem that populates the field.) We might do that as part of this issue or as a follow-up.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@D-XPERT:

Thanks for looking at the eslint errors. I left a few comments on the MR. If you have a chance, please respond to them. I am setting the issue status back to NW for that.

Your commit messages were brief. Could you say a little more? It looks as though the first commit might have been automated fixes, and the second commit was manual. Is that right?

Meanwhile, I have ignored the remaining phpcs error, fixed my broken phpstan configuration, and updated a test to match the changes in the code. I think this issue is almost done!

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I fixed the one problem reported by stylelint.

I would rather let someone more familiar with JS take a look at the eslint results.

I am still doing something wrong with phpstan.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I set up GitLab CI. You can see the results on the MR: https://git.drupalcode.org/project/conditional_fields/-/merge_requests/41.

I fixed most of the errors and warnings reported by phpcs.

I added a baseline for phpstan, so that we can try to avoid new errors and eventually fix the existing ones. I used the one generated as an artifact from the GitLab CI run. But that does not seem to be working, so I am setting the issue status to NW.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@colan:

Thanks. I will ping @diqidoq and @heddn and try to coordinate with them. Either way, I will see if we can make some progress towards a full release.

I am also adding πŸ“Œ Enable testing with GitLab CI Active to the roadmap for a beta release.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@dww, thanks for the quick updates!

Personally, I do not like "... for more". How about "... for details"?

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I also fixed a deprecation that I noticed when running the test locally. See the change record New API for defining field type categories β†’ .

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

MR 7 is ready for review.

In addition to what I wrote in the issue description, I made a little fix to the functional test. I am not sure why, but the test passes either way when I run it locally, but it failed on GitLab CI before my fix.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I am updating the category: this issue is a bug report, not a support request.

I agree that the priority is Critical. Perhaps the current version will work on a site that is upgrading from an earlier version, and already has one or more serial field. But if a site adds this module and tries to create a serial field, it will not work.

I vote +1 for the fix in the MR. I am updating the MR with a test-only change, so I am setting the status back to NR.

My change is to use the admin UI instead of the API to add a field in the functional test. When I exercise the test locally, it passes. If I revert the fix, then it fails. I am a little surprised that it fails with a different error message:

Drupal\Core\Entity\EntityStorageException: 'field_config' entity with ID 'entity_test.entity_test.field_serial' already exists. in Drupal\Core\Entity\EntityStorageBase->doPreSave() (line 519 of core/lib/Drupal/Core/Entity/EntityStorageBase.php).

For reference, here are the top few lines of the stack trace:

Drupal\Core\Entity\EntityStorageBase->save(Object) (Line: 257)
Drupal\Core\Config\Entity\ConfigEntityStorage->save(Object) (Line: 354)
Drupal\Core\Entity\EntityBase->save() (Line: 609)
Drupal\Core\Config\Entity\ConfigEntityBase->save() (Line: 42)
serial_field_config_create(Object)

FWIW, it is not clear that the problem described in the issue summary is the same as the one discussed in Comment #2 and following.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@colan:

I am willing to be added as a maintainer (or co-maintainer).

The project page mentions the #conditional_fields Slack channel and describes @diqidoq and @heddn as the active maintainers. Is that still accurate?

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

Usability review

We discussed this issue at πŸ“Œ Drupal Usability Meeting 2024-02-23 Active . That issue has a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @AaronMcHale, @benjifisher, @ckrina, @rkoller, @simohell, @skaught, and @worldlinemine.

It looks as though the last time this part of the form was updated was #2098047: Installer ui text cleanup β†’ . That issue removed some information that seemed like "too much".

Besides trying to avoid confusion, it is important to let people know what sort of information is being collected. On that basis, we think that the proposed resolution is a good idea.

While reviewing the form, we saw a few other things that we would like to improve. Some of the changes seem reasonable to do as part of this issue, and others can be done as follow-up issues. We leave that decision to the people working on this issue.

  1. The help text applies to the first checkbox, so it should be moved to a description of that option, rather than a description of the fieldset.
  2. We usually try to avoid the passive voice ("information ... is sent to"). Ideally, the sentence should be rewritten to indicate who or what is sending the information. But we do not have suggested wording, and this does not seem like a particularly bad example of passive voice.
  3. For accessibility, the link text should provide enough context to suggest where the link goes. We felt that "checking for updates" does not meet that requirement. We are open to other suggestions, but perhaps the simplest solution is to add another sentence, using the title of the target page as the link text: "See the Update module overview β†’ for more information." Or maybe use Configuring Update Manager During Installation β†’ as the link (going to that section of the page).
  4. The link to drupal.org does not seem useful, and giving users two links gives them an extra decision to make: which link to follow. Let's remove the link to drupal.org. (Also, that link does not include the "www.", so it leads to a redirect.)
  5. The documentation page already says, "... in order to provide this information, anonymous usage statistics (consisting of a unique key and a list of versions of the software your site is running) are sent to Drupal.org". The new section should repeat that or refer to it, in order to explain what "anonymous information" means. The text you added includes, "... your site automatically checks for the latest updates, keeping your site secure and up to date without manual intervention", which is not accurate.
  6. In order to avoid confusion, the documentation page should use the same terms as the installation form: "Site maintenance account" instead of "site administrator" or "admin user".
  7. The new section of the documentation page should focus on the fact that the options on the installation form enable and configure the Update Manager module. In particular:
    • In the introductory paragraph, explicitly state that the choices can be changed after the installation process.
    • Do not repeat information on what the options do, such as how they are important for security.

If you want more feedback from the usability team, a good way to reach out is in the #ux channel in Slack.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The list of attendees looks right to me.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I notice two hings:

  1. There are 4 RBC issues for the 4.x branch, one of which was last updated in 2022.
  2. The issue summary lists just 3 issues that need to be fixed before releasing a beta version. From the comments, one or two of those may be ready to be closed as outdated.

Maintainers: what can I do to help move this module towards a beta and full release? Would you like me to re-review the RTBC issues? Would you like me to work on the issues listed in the issue summary? Would you like a new issue to enable testing with GitLab CI?

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

From Comment #11:

If I remove those lines from the config schema at the tip of 3.0.x branch I resolve the seg fault locally / OOM in ci

That cannot be the right fix. I tried removing those lines and ran the PHPUnit tests:

There was 1 error:

1) Drupal\Tests\rest_views\Functional\RestViewsTest::testRestView
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for views.view.rest_views_test with the following errors: views.view.rest_views_test:display.default.display_options.fields.nid_export.type missing schema, views.view.rest_views_test:display.default.display_options.fields.uid_export.type missing schema

While I was running tests, I also noticed some deprecation notices, so I added πŸ“Œ Normalizers/Denormalizers should implement ::getSupportedTypes() instead of ::hasCacheableSupportsMethod() Active .

From Comment #14:

I've been looking at rest_views_modules_installed for a bit, it's used to automatically install required submodules when the contributed modules are there.

I have been looking at it, too. It is problematic.

The idea is to enable any of the three submodules if one of its dependencies is being enabled, and the rest of its dependencies are already installed (or are being installed). IMO this is a bad idea. Calling $moduleInstaller->install() from an implementation of hook_modules_installed() is just asking for trouble (like infinite recursion).

But that function does not cause any trouble for me, even when I try to reproduce the bug described in this issue. (I installed the Umami profile, since it is multilingual. Then I installed rest_views, added a view that uses it, and installed entity_reference_revisions.) This is why:

      foreach ($info['dependencies'] as $dependency) {
        $dependency = explode(' ', $dependency)[0];

Those lines have not been updated since c9a7ae1 (no issue on d.o), which replaced "views (>=8.7)" (note the space) with "drupal:views" (among other changes).

Currently, that function does not do anything.

@pwolanin, if you have any patches you are applying that you neglected to mention, then you owe us a face-palm emoji. Can you test whether removing the whole function fixes your problem? Or is that more or less what you did in Comment #5?

@nicxvan, what do you want to do about this problematic function?

  1. Get rid of it.
  2. Convert it to showing a message instead of auto-installing the submodules.
  3. Fix it so that it auto-installs the submodules.

Since the function is currently broken, (1) is not such a bad idea. I guess that (2) is more friendly.

If you do want to fix it (Option 2 or 3), then there may be some other problems. It would be nice (Option 2) or a really good idea (Option 3) to move the code into a service class so that we can inject the module services, mock them, and test the code.

I will have a look at the locale module, following up on Comments 8-10.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

In further testing, I noticed that I can reproduce the problem using the Standard profile if I enable the field_layout and layout_builder modules.

I also noticed what @Anybody meant in the issue summary:

This should also solve the side-effect of layout_builder_blank appearing in selects!

With 11.x, I do not see that option, but if I fix the plugin annotations and visit /admin/structure/types/manage/article/form-display, then layout_builder_blank appears in the Layout settings form near the bottom of the page. I think this counts as a regression, which means we cannot defer it to a follow-up issue. I am reverting the issue title to what it was before Comment #47 and removing the tag for a follow-up. While I am at it, I am changing the issue priority.

I already updated MR 6533. I think this is the least disruptive fix: call getFilteredDefinitions() in LayoutPluginManager::getLayoutOptions(). The only problem is that we do not know which $consumer to pass as the first argument, so I just use $this->getType(). This ends up invoking both hook_plugin_filter_layout_alter and hook_plugin_filter_layout__layout_alter.

We should consider other approaches:

  1. Add an optional parameter to LayoutPluginManager::getLayoutOptions() and pass it to getFilteredDefinitions(), using $this->getType() as a fallback.
  2. Make $consumer optional, and invoke just the first hook if it is not provided.

I will mention these options in the issue summary. Maybe the safest approach is to use the current fix and then implement (1) in a follow-up.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I updated some formatting and added issue credits. Someone should check the credits before we declare this issue Fixed.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@joelseguin: You are welcome. Technically, I just created the issue and reviewed the patch (or the merge request). Thank you for bumping this issue.

@IgorMashevskyi: Sorry, I did not notice that you updated the MR. I regularly check for issues that have been updated, but not MRs. I will set the status to NR now, and I will try to have another look soon.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

In the issue summary, I mentioned that this issue is a follow-up to πŸ“Œ Make CI template compatible with private repositories Fixed . I am adding that as a related issue.

Thanks for the link to πŸ› Test-only job fails with "couldn't find remote ref refs/heads/11.x" when 11.x branch does not exist in fork Needs review . I think that is a duplicate of this issue, but if that is already RTBC, we can close this one when it gets fixed. I am adding it as another related issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

For purposes of this issue, we should probably add a test that will fail if the no_stub configuration is removed from these migrations.

I think we should also add documentation for how it works currently. For instance, the doc block for the migration_lookup plugin should mention that the candidates for stub migration might not be willing to serve. And Migration documents its definition keys, including destination. It should also document the no_stub option there.

Then we can move the discussion of whether to deprecate this feature to #3247754: [PP-1] Remove use of MigrateSkipRowException from core process plugins β†’ and related issues.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@tawellman, thanks for posting the stack trace in Comment #27. That shows the (core, experimental) field_layout module. When I enable that, I can reproduce the error by visiting /en/admin/structure/types/manage/article/form-display (Umami demo profile) as you suggested.

I have updated the steps to reproduce on that issue, and I am closing this one as a duplicate.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

In #3315678-27: strnatcasecmp(): Passing null to parameter #2 ($string2) of type string is deprecated β†’ , @tawellman posted a stack trace that showed the (core, experimental) field_layout module. Using that, I can reproduce the error using just Drupal core. I am updating the steps to reproduce in the issue summary here, closing the other issue as a duplicate, and giving credit to @tawellman on this issue.

It might be appropriate to give credit to some of the others who commented on that issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

Try the latest patch on πŸ› Deprecated function: strnatcasecmp(): Passing null to parameter #1 ($string1) of type string is deprecated in LayoutPluginManager Needs review .

From the issue summary:

In my Drupal 10 installation, if I go to "Edit" for any paragraph, ...

Do you mean "paragraph type" rather than "paragraph"? See the updated steps to reproduce on the core issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

On second thought, the @param comment for getSortedDefinitions() comes from the interface, so it is not so easy to make it more specific. Maybe we should, but not for just this plugin type.

I added annotations for label and category. I considered adding a description, since getDescription() has the same return type declaration, but the annotation class specifically says that $description is optional. And then there is $templatePath. (shrug)

I am updating the steps to reproduce and the proposed resolution in the issue summary.

I am also attaching a patch for convenience.

I am removing the "Needs tests" issue tag. I do not think we need to test this.

BTW, @longwave commented in #3316811-8: strnatcasecmp(): Passing null to parameter #1 ($string) of type string is deprecated β†’ :

It appears there is a plugin with a NULL label somewhere, it would be good to know exactly which plugin that is, and whether it is a core issue, or contrib or custom code.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@Anybody:

Thanks for the detailed analysis in the issue summary. I agree that casting to string or using the null-coalesce operator would be fixing the symptom instead of the root cause.

The problem with using getFilteredDefinitions() in LayoutPluginManager is that you are specifying $consumer that works for this case, but that Core class can be used anywhere in Drupal. When the block module calls getSortedDefinitions(), why should it invoke the hook from the layout_builder module? Related: when I search for getFilteredDefinitions(), I find that it is called in a few placed in the layout_builder module.

I think you missed one point: LayoutDefinition::getCategory() declares its return type as string|\Drupal\Core\StringTranslation\TranslatableMarkup. It is not allowed to return null. So adding a category to the BlankLayout plugin annotations is the correct fix.

I will prepare a merge request that does that. I will also add any other annotations that seem to be required, and make the @param comment for getSortedDefinitions() more specific.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The config schema is mentioned in this comment: #3338083-3: Create functional test for basic view functionality. β†’ .

I would still like to have steps to reproduce. From #7:

It seems like the problem is locally caused by a view (config) that uses rest_views and also views_streaming_data to handle a csv export of the data.

Is that all it takes? Can you share the config for that view?

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@dww

I am happy to leave the decisions to the maintainers. I just want to make sure that you read the earlier comments on this issue about whether to keep or drop support for D9 and for PHP 7.

Also, this is one of the issues listed as a "Must have" on 🌱 Inline Entity Form stable release plan Active . If this issue is postponed, then please update the plan issue.

It is OK with me if you postpone this issue until IEF 4.0. It is also OK with me if you drop support for D9 and PHP 7 in the 3.x branch and keep it in the 1.x branch.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@pwolanin:

You have not given enough information to reproduce the problem, so I am setting the status to Postponed (MNMI). What other modules are involved?

It is hard to see how this module could cause problems during the installation phase. It does not have a .install file, and it does not define any entities, so I cannot think of anything that happens at install time. Config import is a different matter.

Perhaps some other module, or Drush, got updated when you updated this module to 3.0.0 from 2.0.1. Can you use composer-lock-diff to get a complete list of modified packages?

Do you see this problem in local copies of your site or only in your CI environment? If it is only in CI, then maybe your jobs are getting confused by the existence of .gitlab-ci.yml.

That last suggestion was a stretch. I will quit now and wait for some more information.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I would use the status "Closed (outdated)" rather than "Fixed".

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

In #3413233-8: Drop PHP7 support to help with code cleanup β†’ , I pointed out that there is another constructor update (from ✨ Allow themes to alter inline entity forms Fixed ) that should have a BC layer. Since we already handle something similar for the MigrationHelper class in this issue, we might want to fix that here.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I rebased on the current 3.x and resolved a merge conflict from πŸ› Field permissions access override Fixed .

I restored compatibility with Drupal 9, following the decision in Comment #6. But then Comment #5 no longer applies, so I added a minimum PHP version in the .info.yml file (but not in composer.json).

I also reviewed the changes made for compatibility with Drupal 10. I did not find any changes that would make this module incompatible with Drupal 9. I did find two things that require further work:

✨ Allow themes to alter inline entity forms Fixed added a parameter to the EntityInlineForm constructor without a BC layer.

There is a comment in InlineFormInterface::getEntityLabels():

* @todo Remove when #1850080 lands and IEF starts requiring Drupal 8.1.x

Since #1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed β†’ was fixed a long time ago, we should get rid of that method instead of updating it.

I am not sure whether to fix those problems as part of this issue or create new issues. We might fix the first one as part of πŸ“Œ Get PHPStan level 1 passing and enable automated test job Active , which already adds a BC layer for the MigrationHelper constructor.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@drunken monkey:

Thanks for catching the version requirement. BTW, Drupal 9.3 also introduced the module-specific permissions form that I mentioned in Step 4 of my testing instructions (Comment #3).

I reviewed the changes, and they look good. To keep myself honest, I checked the test to see where the magic string "muh" is introduced.

I repeated the test from Comment #3 and got the same result.

The other issue brought this bug to my attention. Reporting a bug is an important step in getting it fixed, even though the initial analysis was incomplete. I reviewed the issue that introduced permission dependencies, and I wrote the change record, so I did not need much help in figuring it out.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

One point that came up in the usability meeting (Comment #26) is that there are use cases for having more than one long-text-with-summary field, or a multi-valued field. I have never seen it, and I do not think it meets the threshold of the 80% use case, but it does come up.

The current link to open the summary form was probably the only way to do it when this form was first designed. That was a long time ago. I think we should use a standard <details> element now that we can.

Probably the summary should be hidden by default, at least when it is empty. That is the least disruptive change from the current behavior.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@simohell:

Thanks. With that and the link to the recording that I am posting now, this issue is done.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area
πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@dww:

Congratulations on/thanks for joining as a co-maintainer of this module! I have a client who would like to see a stable release, so you can ping me in Slack if there is a task you would like me to do on the way there.

I reviewed the two commits that you added, and I updated the change record https://www.drupal.org/node/3417929 β†’ to match. LGTM

I am in the same boat as you: I have not stuck to the reviewer role on this issue. But if we are allowed to review each other's work, then I think we can (jointly) declare this issue to be RTBC.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

The MR is ready for review.

There are one or two open threads on the MR, but I think the intention is to open follow-up issues for them, As far as I am concerned, we do not need any more code changes for this issue.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

Make the variable name match.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I rebased and resolved merge conflicts caused by πŸ“Œ Fix the issues reported by phpcs Needs work .

I made some changes to MigrationHelper in order to keep phpcs happy.

I am going to provide a BC layer, following Drupal deprecation policy > Constructor parameter additions β†’ .

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

benjifisher β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@smustgrave:

Yes. Thanks for asking.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@mikelutz:

The MR needs a rebase after ✨ Allow process plugins to stop further processing on a pipeline RTBC . I also made some small comments on the MR. Please set the status of this issue back to NR when you think it is ready. (I am not sure whether you forgot to do that or if you meant to continue working on the MR before getting more feedback.)

The refactoring is hard to review, but the result is worth it!

I think the "Needs subsystem maintainer review" tag was for the general approach. I am removing it now. The general approach is good, and we are now fighting with the smaller details.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@Akhil Babu:

Thanks for catching that!

Instead of

      if ($connection->getConnectionOptions() == Database::getConnectionInfo()['default']) {

we should probably use

      if ($connection->getConnectionOptions() === Database::getConnection()->getConnectionOptions()) {

I tested this using both MariaDB and SQLite.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

I think it is worth opening a feature request for the core entity system: add a way to indicate that a string is already serialized, so it should be stored as is. If that feature gets added, then we can easily update the Migrate API to add support for it. I have not checked myself: I am trusting you that there is not already such an option.

Until that happens, the current approach seems like a reasonable work-around.

From #8:

Also, we should probably pass allowed_classes = TRUE to unserialize() for security.

From https://www.php.net/unserialize:

Omitting this option is the same as defining it as true: PHP will attempt to instantiate objects of any class.

Did you mean to say FALSE instead of TRUE?

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@duncancm, thanks for adding the links.

I already commented on πŸ“Œ Allow user to add display modes from respective field UI's. Needs work . I am attaching a rough transcript and a link to the recording on YouTube.

πŸ‡ΊπŸ‡ΈUnited States benjifisher Boston area

@ddavisboxleitner:

Thanks for getting the transcript started, and bumping this issue that we missed.

I cleaned up some of the formatting and assigned issue credits.

Production build https://api.contrib.social 0.61.6-2-g546bc20