🇺🇸United States @clayfreeman

This can already be done via the drush config:set command. I'm not sure there's a good argument to be made for adding a custom command for this purpose.

I'm not sure this bot is accurate; I'm not seeing any specific CSpell errors, and none were highlighted in CI anyway.

Since this is a test-only nit, moving back to RTBC.

I believe the root of the problem is that the Turnstile library is being attached directly to the HTML head, which doesn't exist for AJAX responses. Instead, I've converted the Turnstile library to a Drupal library and added it as a dependency of the element. This solves the problem for my dynamically-loaded form.

clayfreeman → made their first commit to this issue’s fork.

Ready for initial review.

clayfreeman → created an issue.

I left some replies. We need to decide first and foremost whether we want to allow HTML tags in the title. If not, then these changes you marked as out of scope can be reverted.

Ready for initial round of feedback.

I figure it would be impossible for rector to catch even a majority of the work that would need to be done because it will largely rely on the developer's diligence to use or notate proper type information; I assume you'd need to be able to establish the ContentEntityBase type via static analysis. Consider even the following scenario with accurate type information:

// This would be missed entirely because we can't leap from EntityInterface to ContentEntityBase.
// I'm also assuming runtime information would be unavailable (e.g., entity type ID).
function hook_entity_presave(EntityInterface $entity) {
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

I assume these scenarios would likely be fine:

function hook_entity_presave(EntityInterface $entity) {
  // Node -> ContentEntityBase
  // Can we make the leap from NodeInterface?
  if ($entity instanceof Node && $entity->field_some_field?->value) {
    // do stuff
  }
}

// Entity type-specific hook with generic interface.
function hook_node_presave(EntityInterface $entity) {
  /** @var \Drupal\node\Entity\Node $entity */
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

// Entity type-specific hook with entity type-specific interface.
// Node -> ContentEntityBase
// Can we make the leap from NodeInterface?
function hook_node_presave(Node $entity) {
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

I assume the following would also be fine, but I'm less certain. Will custom entity interfaces be an issue (e.g., class Custom extends ContentEntityBase implements CustomInterface)? Would we lose out on detecting CustomInterface, have false positives, ...?

// With a little bit extra work, we can intuit more specific types for some hooks:
// hook_ENTITY_TYPE_presave -> Custom -> ContentEntityBase
function hook_custom_presave(EntityInterface $entity) {
  if ($entity->field_some_field?->value) {
    // do stuff
  }
}

This change almost begs for the magic methods to be part of some interface that EntityBase implements. This would require a minimum of two major releases for BC concerns as far as I know.

These assumptions could be completely wrong, and if they are, then I will happily reassess.

This gets a -1 from me, at least in its current form. We use __get() in all of our projects as the primary method for retrieving entity fields because of how brief it is, so this would be a very time consuming change for us to have to deal with.

One alternative that seems interesting to me is to simply make __get() work only with fields. We could alias __get() to get(), but one distinction that immediately comes to mind between them is that the former doesn't throw an exception when the field doesn't exist. This is behavior that we rely on fairly heavily since it provides a simple, safe way to access fields when you don't care if they exist:

// Currently, this is possible:
if ($entity->field_fizz_buzz?->value) {
  // do something
}

// What it would become:
if ($entity->hasField('field_fizz_buzz') && $entity->field_fizz_buzz->value) {
  // which is needlessly more verbose...
}

// This is slightly better:
if ($entity->get('field_fizz_buzz')->value) {
  // but it's *not* safe if the field doesn't exist; you'll get an exception
}

This seems rather nit-picky, but when multiplied 1,000 times across a project it becomes very time consuming.

I understand the benefit that type inference would bring, but this would be a huge cost for existing projects.

clayfreeman → created an issue.

Great, glad you were able to figure it out. I don't think I want to build a mechanism for detecting these dangling revisions at this time, as it falls outside of the scope of concern of this module.

Closing this out for now since there's no action item.

I should note that for revisionable entity types, the list of IDs returned will be revision IDs instead of entity IDs. I wonder if you may have some dangling revisions in your database for content which has since been deleted?

If you run the below code, you can get a list of uninitialized entities for each entity type:

function get_all_uninitialized_entity_ids() {
  /** @var \Drupal\cached_moderation_state\BatchUpdateHandlerInterface */
  $batch_update_handler = \Drupal::service('cached_moderation_state.batch_update_handler');
  /** @var \Drupal\cached_moderation_state\FieldConfigHandlerInterface */
  $field_config_handler = \Drupal::service('cached_moderation_state.field_config_handler');

  $entity_ids = [];
  foreach ($field_config_handler->getModeratedEntityTypes() as $entity_type) {
    $bundles = $field_config_handler->getModeratedBundlesForEntityType($entity_type);
    $entity_ids[$entity_type->id()] = $batch_update_handler->getEntitiesForEntityType($entity_type->id(), $bundles, TRUE);
  }

  return $entity_ids;
}

echo '<pre>' . \htmlentities(\var_export(get_all_uninitialized_entity_ids(), TRUE)) . '</pre>' . \PHP_EOL;

Unfortunately, I can't help diagnose the issue that's preventing these from being initialized. Typically there's a hook implementation that prevents the normal operation of this module.

If you find out something interesting, feel free to report back.

I should also note, this can be done entirely without a patch now with #309040: Add hook_requirements_alter() → .

I don't believe the follow-up is needed, or at least it shouldn't block the work being done here. I'm happy to entertain the proposed change if Drupal core is willing to take on such a task, but I doubt the likelihood of that outcome.

I'll post in #accessibility -- thanks for the suggestion.

Wouldn't it be much better to simply remove this patch and allow downstream consumers to decide whether they want it or not? For our use case, this patch doesn't really matter and only causes maintenance headaches.

clayfreeman → created an issue.

Pipeline seems to have passed; ready for manual review now.

Would appreciate an expert's review to ensure the work is sufficient :)

clayfreeman → created an issue.

clayfreeman → changed the visibility of the branch 3430739-manual-updates to active.

clayfreeman → changed the visibility of the branch 3430739-manual-updates to hidden.

clayfreeman → made their first commit to this issue’s fork.

clayfreeman → made their first commit to this issue’s fork.

clayfreeman → made their first commit to this issue’s fork.

I think I'm okay with this module acting after the page cache. We can't control page caching that happens outside of Drupal anyway, and I'd rather avoid introducing a discrepancy in behavior based on where the cached page comes from.

This module's restrictions are applied site-wide and do not target specific forms such as the registration and login forms. If you have any forms other than the registration and login forms that you want to allow an international audience to use, this module would not be a good fit for that use case.

Assuming you don't mind restricting form submissions on a site-wide basis, you can configure Geoblock's Applicable HTTP methods to suit your needs. If GET is not selected for this field, then Geoblock won't restrict an international audience from simply browsing the site.

Thanks for your contribution! I went ahead and tagged a patch release for this as well. Should be available for installation shortly.

I'm experiencing something similar. We had the comment module installed (via the standard install profile in Drupal core) and removed it recently because of the security advisory related to that module. This caused some roles to be updated to remove permissions that were provided by that module. As a result, views which used role-based access control and referenced a role that was updated were deleted. We were able to import these views without modification using Configuration > Development > Config synchronization > Import.

+++ b/src/Plugin/views/style/Zodiac.php
@@ -203,6 +211,12 @@ class Zodiac extends StylePluginBase implements ContainerFactoryPluginInterface
+        '#description' => $this->t('The text template used for the live region updates ...'),

Add punctuation to the end of this description.

I'm not sure if these test failures are real or not; they all pass locally except for GeolocationGeometryViewsBoundaryTest, which was already failing before I made any changes. Passing for review.

clayfreeman → created an issue.

clayfreeman → changed the visibility of the branch 3409805-allow-certain-entity to active.

clayfreeman → changed the visibility of the branch 3409805-allow-certain-entity to hidden.

clayfreeman → created an issue.

This issue introduces a major bug that breaks AJAX for form elements without a #submit key:

TypeError: in_array(): Argument #2 ($haystack) must be of type array, null given in in_array() (line 675 of /code/web/modules/contrib/token/token.module)

#0 /code/web/modules/contrib/token/token.module(675): in_array('::save', NULL)
#1 [internal function]: token_node_menu_link_submit('node', Object(Drupal\node\Entity\Node), Array, Object(Drupal\Core\Form\FormState))

The condition also needs to check whether the key exists, otherwise we may pass NULL to in_array().

This patch addresses a couple minor UX concerns with the value callback in prior iterations.

Oops, one last test fix.

Test this patch and let me know if it meets your needs. If so, I'll schedule a feature release for early July.

It seems that the project to which you refer in the issue description forked this project:

This simple module forked from Entity Bundle Permissions → but much more useful.

I'm not sure why the author of this downstream module decided to fork this project, but if you think these projects should be merged, then I would encourage you to open an issue on the downstream project to inquire about why a fork was necessary since that was not my decision.

This project will not add support for PHP 7 or Drupal 8, since both are EOL. My company will not spend time to support EOL software. If the primary feature request is to suppress excess permission checks and generation, you will need to adjust the scope of this issue or open a separate issue for our consideration.

Defining a menu link in the administration menu would require this module to establish a potentially disruptive, implicit dependency on Drupal's current menu structure -- in this case, the plugin ID of a given parent menu item provided by Drupal core. While the backward compatibility policy does state that "plugin IDs [...] can be relied upon," I've chosen to avoid this implicit dependency.

This module instead leverages the configure key in its *.info.yml, which adds a "Configure" link to the module's description on the "Extend" page. Should Drupal choose to forgo this concept in the future, there is no potentially disruptive dependency relationship to worry about.

Reliability is a primary priority for this module. I may be paranoid in making this decision, but given that unintended side effects could result in an inaccessible site, I want to reduce the incidence of these types of scenarios as much as possible.

All that being said, end users who need a menu link can still add one manually.

I am unable to reproduce the issue. How are you installing the module? If you're installing without using Composer, have you made the module's dependencies available? Namely, this module requires league/iso3166 for a canonical list of country codes.

This project uses a very strict interpretation of Drupal's backward compatibility policy → which prevents us from leveraging Drupal's administration menu out of the box. If you wish to add an item to Drupal's administration menu, you can still do so manually by navigating to Structure > Menus > Administration > Add link and using the following path: /admin/config/geoblock.