🇺🇸United States @thhafner

thhafner → created an issue.

Tested on 11.x with the language switcher use case in the initial issue description and functioning as expected.

thhafner → created an issue.

One more time with the correct patch file.

adding correct patch file.

Encountering a bug in this patch where ANY url that contains 9 consecutive digits is transformed into a tel link. Propose changing regex to something that does NOT match 9 digits without anything else.

For example,
^(\+\d{1,2}\s)?\(?\d{3}\)?[\s.-]\d{3}[\s.-]\d{4}$

matches
123-456-7890
(123)-456-7890
(123) 456 7890
123 456 7890
123.456.7890
+91 (123) 456-7890

among other combinations

I understand phone number matching opens up an entire can of worms, but this is what we are currently using to prevent erroneous tel links.

thhafner → created an issue.

https://www.drupal.org/project/responsive_views_grid →

I tested with the ternary operator and it works as expected with s3fs. I have not validated it with a standard public filepath yet.

@jmdeleon need to use a ternary operator instead of null coalescing as realpath returns a string or FALSE not NULL when it cannot find the file.

https://www.php.net/manual/en/function.realpath.php#refsect1-function.re...

thhafner → created an issue.

For completeness, this issue describes the problem and solution very well.

https://www.drupal.org/project/s3fs/issues/3352993#comment-15002764 💬 Is it expected that s3fs will override Drupal\Core\File\FileSystem service? Fixed

thhafner → created an issue.

Patch works as expected for me.

thhafner → created an issue.

Agreed with above posters. This issue isn't really fixed until there is an actual release of the module with patch #16 🐛 Fix TypeError: array_keys(): Argument #1 ($array) must be of type array, null given in Language Drop down Block class Fixed included.

thhafner → created an issue.

Patch for #88 ✨ Implement a generic revision UI Fixed did not upload correctly. Reuploading.

Pushing this issue along a bit. Made updates that do not require a great deal of additional discussion.

Remaining Tasks:

1. +++ b/core/lib/Drupal/Core/Entity/EntityBase.php
   @@ -196,7 +196,7 @@ public function toUrl($rel = 'canonical', array $options = []) {
   -    if ($rel === 'revision' && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   +    if (in_array($rel, ['revision', 'revision-revert-form'], TRUE) && $this instanceof RevisionableInterface && $this->isDefaultRevision()) {
   

   The patch over at #2927077: $entity->toUrl('revision-*') → should fill revision parameter on all applicable routes. has looser logic and checks any link that starts with 'revision':

   > However, entity annotations can provide other revision-specific routes, such as "revision_revert" or "revision_delete"

   Should that apply here too?

2. private function getHandlerClassWithImplementationChecks(EntityTypeInterface $entity_type, $handler, $implementing_class) {
   

   I'm confused by why we need this helper method. There's perhaps a case to be made for all entity handlers to get checked against the interface they ought to be implementing. But that's a wider issue. (And also one that leads to other issues to with declaring handler types in a formal manner, so we can declare the interface they need to have.)

3. if ($entity_type->hasLinkTemplate('version-history') && $version_history_class = $this->getHandlerClassWithImplementationChecks($entity_type, 'version_history', VersionHistoryControllerInterface::class)) {
   

   This pattern isn't used by the other entity route providers -- they hardcode the controller class they use for their routes. I think it's best not to invent too many new patterns at once.

   There's a case to be made in a follow-up for introducing a "controllers" annotation that mirrors the "route_provider" annotation, as I for one have often subclasses route providers just to switch the controller class set in route callbacks.

4. Also, I'm not seeing a test entity type with a 'revision_revert' form class defined, so it looks like the test coverage is incomplete.

5. public function access(Route $route, AccountInterface $account, RouteMatchInterface $route_match = NULL) {
   

   Rather than inheritdoc, follow the pattern of EntityAccessCheck and document what this access checker expects in the route.

6. protected function checkAccess(ContentEntityInterface $entity, AccountInterface $account, $operation = 'view') {
   

   Missing docs.

7. operation = $route->getRequirement('_entity_access_revision');
   

   AFAIK, an access checker needs to be declared as a service, with the requirements key, '_entity_access_revision' in our case, also declared in services.yml. E.g.:

   access_check.entity_revision:
     class: Drupal\Core\Entity\EntityRevisionAccessCheck
     arguments: ['@entity_type.manager', '@current_route_match']
     tags:
       - { name: access_check, applies_to: _entity_access_revision }
   

   Without this, I get an access denied on the routes.

   Though, why do we need @current_route_match as an injection?

8. if (empty($route_match)) {
     $route_match = $this->routeMatch;
   }
   

   Why do we need this? The access() method gets it as a parameter.

9. * @return string
    *   Returns a string to provide the details of the revision.
    */
   abstract protected function getRevisionDescription(ContentEntityInterface $revision, $is_current = FALSE);
   

   but VersionHistoryController::getRevisionDescription() doesn't return a string but an array.