krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
Patch works.
The current implementation on the MRs does the account switching in an event subscriber. This, however, causes a problem in that
switching back will not happen if there's an error in the following cases:
- If an operation is cancelled by a pre-initiate subscriber.
- If there's a fatal error in another pre-initiate subscriber.
- If there's a fatal error in another post-terminate subscriber.
Here's example code.
$cancel = $this->preInitiate(
Events::REMOTE_LIST_PRE_INITIATE,
'import_list',
$context,
$sync
);
if ($cancel) {
return;
}
try {
$this->doImportRemoteList($sync, $filters, $options, $context);
}
finally {
// Notify subscribers that the operation has terminated.
$this->postTerminate(
Events::REMOTE_LIST_POST_TERMINATE,
'import_list',
$context,
$sync
);
}
We therefore need to move account switching from an event subscriber to a service that is used by the import/export manager.
- Move the logic to a service called AccountSwitcher
- In all relevant places in both the import and the export manager, switch accounts before pre-iniate events.
- Wrap pre-initiate events in try/catch, switch back accounts, re-throw the error.
- Switch back accounts if the operation is cancelled.
- Wrap post-terminate events in try/catch, switch back accounts, re-throw the error.
Now, here's the other complexity. An operation can have dependent operations i.e. it can call or be called within the exeuction of another operation. That's actually a pretty common use case.
Imagine the following:
- Operation A switches from user 0 to user 1.
- Operation A runs.
- Before terminating, operation A triggers the execution of operation B.
- Operation B switches from user 1 to user 12.
- Operation B runs.
- Operation B terminates, at which point needs to switch back from user 12 to user 1.
- Operation A terminates, at which point needs to switch back from user 1 to user 0.
We therefore need to track in a variable whether the account switched and then use that result to switch back.
Ok got it, thanks for the explanation. I'm not sure really how common this problem is, but I guess I'm old school and prefer to manually craft code. I also understand the "lazy developer" syndrome where people want to focus on the actual logic that matters most and have their text editor do the boring stuff for them - not saying this in a demeaning way, I appreciate the practicality of such attitude. I write scripts to automate things for me all the time.
In that case I think it should be a suggestion and not phrased with MUST in the standards, there could be a default Sniff in Coder but also allowing for alternatives letting the maintainer of the code make a different choice.
The way Git merges changes does not have much to do - if anything - with the sorting of the contents of the code. Conflicts most commonly happen when you edit the same lines.
5. use Drupal\address\AddressInterface;
6. use Drupal\node\NodeInterface;
7. use Drupal\user\UserInterface;
Branch A adds:
5. use Drupal\address\AddressInterface;
+ 6. use Drupal\group\GroupMembershipLoaderInterface;
7. use Drupal\node\NodeInterface;
8. use Drupal\user\UserInterface;
Branch B adds:
5. use Drupal\address\AddressInterface;
+ 6. use Drupal\commerce\ConfigurableFieldManagerInterface;
7. use Drupal\node\NodeInterface;
8. use Drupal\user\UserInterface;
The result:
use Drupal\address\AddressInterface;
<<<<<<< Branch A
use Drupal\group\GroupMembershipLoaderInterface;
=======
use Drupal\commerce\ConfigurableFieldManagerInterface;
>>>>>>> Branch B
use Drupal\node\NodeInterface;
use Drupal\user\UserInterface;
Both changes placed the import statements where they should and followed the correct alphabetical sorting. Note that I actually tested this to make sure I'm not saying anything inaccurate and got the behavior above.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
So, what is the purpose of sorting use statements? Is it to please PHPStorm or X, Y or Z text editor so that when you use its sorting feature it is sorted the way the editor wants? And then run PHPCS and get it to give you green lights so that the pipeline passes? Or is it to randomly choose an algorithm i.e. case-sensitive or case-insensitive and agree that everybody follows it?
To me, no, it's about human readability. Even that, using find/search functionality in most text editors you can easily find a use statement directly without having to look for it with the eye. So, the most important circumstance where it matters is when you want to review all use statements in a file, or to find a statement that you don't quite remember what exactly it is and can't use search e.g. I don't know which PSR-18 HTTP client is being used so I can't search specifically for "Guzzle" to find the import statement - I need to look for it manually.
That is, we should be taking an approach based on how the human eye/brain would more comfortably and quickly read the use statements, not how a software or algorithm would do. The software does not care, give it a rule and it will follow it.
What I have concluded to work best is to follow a more intelligent approach and use logical groups of statements separated by an empty or comment line. Do that only if the number of statements justify it - otherwise it's not necessary e.g. if it's more than 5. And if the number of statements in a group grows too much, break it further into subgroups.
Note that PSR-12 might not define the sorting algorithm to use, but it does define as a MUST to group use statements for classes, functions and constants.
I find it really easy for the eye to find what it's looking for when I follow this method.
Here is an example from group.module:
use Drupal\Component\Utility\Html;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Config\Entity\ConfigEntityInterface;
use Drupal\Core\Database\Query\AlterableInterface;
use Drupal\Core\Database\Query\SelectInterface;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\Core\Field\FieldItemListInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element;
use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\Core\Session\AccountInterface;
use Drupal\group\Entity\GroupRelationshipInterface;
use Drupal\group\Entity\GroupInterface;
use Drupal\group\Entity\Storage\ConfigWrapperStorageInterface;
use Drupal\group\Entity\Storage\GroupRelationshipStorageInterface;
use Drupal\group\Entity\Storage\GroupRoleStorageInterface;
use Drupal\group\QueryAccess\EntityQueryAlter;
use Drupal\group\QueryAccess\GroupRelationshipQueryAlter;
use Drupal\group\QueryAccess\GroupQueryAlter;
use Drupal\views\Plugin\views\query\QueryPluginBase;
use Drupal\views\Plugin\views\query\Sql;
use Drupal\views\ViewExecutable;
I would write this as follows:
// Drupal modules.
use Drupal\group\Entity\GroupRelationshipInterface;
use Drupal\group\Entity\GroupInterface;
use Drupal\group\Entity\Storage\ConfigWrapperStorageInterface;
use Drupal\group\Entity\Storage\GroupRelationshipStorageInterface;
use Drupal\group\Entity\Storage\GroupRoleStorageInterface;
use Drupal\group\QueryAccess\EntityQueryAlter;
use Drupal\group\QueryAccess\GroupRelationshipQueryAlter;
use Drupal\group\QueryAccess\GroupQueryAlter;
use Drupal\views\Plugin\views\query\QueryPluginBase;
use Drupal\views\Plugin\views\query\Sql;
use Drupal\views\ViewExecutable;
// Drupal core.
use Drupal\Component\Utility\Html;
use Drupal\Core\Access\AccessResult;
use Drupal\Core\Config\Entity\ConfigEntityInterface;
use Drupal\Core\Database\Query\AlterableInterface;
use Drupal\Core\Database\Query\SelectInterface;
use Drupal\Core\Entity\EntityInterface;
use Drupal\Core\Field\FieldDefinitionInterface;
use Drupal\Core\Field\FieldItemListInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element;
use Drupal\Core\Routing\RouteMatchInterface;
use Drupal\Core\Session\AccountInterface;
Here is an example from Drupal\graphql_core_schema\Plugin\GraphQL\Schema\CoreComposableSchema:
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\File\FileSystemInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element\Checkboxes;
use Drupal\Core\TypedData\TypedDataTrait;
use Drupal\graphql\GraphQL\ResolverBuilder;
use Drupal\graphql\GraphQL\ResolverRegistry;
use Drupal\graphql\GraphQL\ResolverRegistryInterface;
use Drupal\graphql\Plugin\GraphQL\Schema\ComposableSchema;
use Drupal\graphql\Plugin\SchemaExtensionPluginInterface;
use Drupal\graphql\Plugin\SchemaExtensionPluginManager;
use Drupal\graphql_core_schema\CoreComposableConfig;
use Drupal\graphql_core_schema\CoreComposableResolver;
use Drupal\graphql_core_schema\CoreSchemaExtensionInterface;
use Drupal\graphql_core_schema\CoreSchemaInterfaceExtensionInterface;
use Drupal\graphql_core_schema\EntitySchemaBuilder;
use Drupal\graphql_core_schema\Form\CoreComposableSchemaFormHelper;
use Drupal\graphql_core_schema\GraphQL\Enums\DrupalDateFormatEnum;
use Drupal\graphql_core_schema\GraphQL\Enums\EntityTypeEnum;
use Drupal\graphql_core_schema\GraphQL\Enums\LangcodeEnum;
use Drupal\graphql_core_schema\SchemaBuilder\SchemaBuilderGenerator;
use Drupal\graphql_core_schema\SchemaBuilder\SchemaBuilderRegistry;
use Drupal\graphql_core_schema\TypeAwareSchemaExtensionInterface;
use Drupal\typed_data\DataFetcherTrait;
use GraphQL\Language\AST\DocumentNode;
use GraphQL\Language\AST\InterfaceTypeDefinitionNode;
use GraphQL\Language\AST\TypeDefinitionNode;
use GraphQL\Language\AST\UnionTypeDefinitionNode;
use GraphQL\Language\Parser;
use GraphQL\Type\Schema;
use GraphQL\Utils\BuildSchema;
use GraphQL\Utils\SchemaExtender;
use GraphQL\Utils\SchemaPrinter;
use Symfony\Component\DependencyInjection\ContainerInterface;
// Drupal modules.
use Drupal\graphql\GraphQL\ResolverBuilder;
use Drupal\graphql\GraphQL\ResolverRegistry;
use Drupal\graphql\GraphQL\ResolverRegistryInterface;
use Drupal\graphql\Plugin\GraphQL\Schema\ComposableSchema;
use Drupal\graphql\Plugin\SchemaExtensionPluginInterface;
use Drupal\graphql\Plugin\SchemaExtensionPluginManager;
use Drupal\graphql_core_schema\CoreComposableConfig;
use Drupal\graphql_core_schema\CoreComposableResolver;
use Drupal\graphql_core_schema\CoreSchemaExtensionInterface;
use Drupal\graphql_core_schema\CoreSchemaInterfaceExtensionInterface;
use Drupal\graphql_core_schema\EntitySchemaBuilder;
use Drupal\graphql_core_schema\Form\CoreComposableSchemaFormHelper;
use Drupal\graphql_core_schema\GraphQL\Enums\DrupalDateFormatEnum;
use Drupal\graphql_core_schema\GraphQL\Enums\EntityTypeEnum;
use Drupal\graphql_core_schema\GraphQL\Enums\LangcodeEnum;
use Drupal\graphql_core_schema\SchemaBuilder\SchemaBuilderGenerator;
use Drupal\graphql_core_schema\SchemaBuilder\SchemaBuilderRegistry;
use Drupal\graphql_core_schema\TypeAwareSchemaExtensionInterface;
use Drupal\typed_data\DataFetcherTrait;
// Drupal core.
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\File\FileSystemInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element\Checkboxes;
use Drupal\Core\TypedData\TypedDataTrait;
// External libraries.
use GraphQL\Language\AST\DocumentNode;
use GraphQL\Language\AST\InterfaceTypeDefinitionNode;
use GraphQL\Language\AST\TypeDefinitionNode;
use GraphQL\Language\AST\UnionTypeDefinitionNode;
use GraphQL\Language\Parser;
use GraphQL\Type\Schema;
use GraphQL\Utils\BuildSchema;
use GraphQL\Utils\SchemaExtender;
use GraphQL\Utils\SchemaPrinter;
use Symfony\Component\DependencyInjection\ContainerInterface;
And since the first group in this example is quite big, I'd break it further down to:
// This module.
use Drupal\graphql\GraphQL\ResolverBuilder;
use Drupal\graphql\GraphQL\ResolverRegistry;
use Drupal\graphql\GraphQL\ResolverRegistryInterface;
use Drupal\graphql\Plugin\GraphQL\Schema\ComposableSchema;
use Drupal\graphql\Plugin\SchemaExtensionPluginInterface;
use Drupal\graphql\Plugin\SchemaExtensionPluginManager;
// Contrib modules.
use Drupal\graphql_core_schema\CoreComposableConfig;
use Drupal\graphql_core_schema\CoreComposableResolver;
use Drupal\graphql_core_schema\CoreSchemaExtensionInterface;
use Drupal\graphql_core_schema\CoreSchemaInterfaceExtensionInterface;
use Drupal\graphql_core_schema\EntitySchemaBuilder;
use Drupal\graphql_core_schema\Form\CoreComposableSchemaFormHelper;
use Drupal\graphql_core_schema\GraphQL\Enums\DrupalDateFormatEnum;
use Drupal\graphql_core_schema\GraphQL\Enums\EntityTypeEnum;
use Drupal\graphql_core_schema\GraphQL\Enums\LangcodeEnum;
use Drupal\graphql_core_schema\SchemaBuilder\SchemaBuilderGenerator;
use Drupal\graphql_core_schema\SchemaBuilder\SchemaBuilderRegistry;
use Drupal\graphql_core_schema\TypeAwareSchemaExtensionInterface;
// Core modules.
use Drupal\typed_data\DataFetcherTrait;
// Core libraries.
use Drupal\Core\Cache\CacheBackendInterface;
use Drupal\Core\DependencyInjection\DependencySerializationTrait;
use Drupal\Core\Entity\EntityTypeManagerInterface;
use Drupal\Core\Extension\ModuleHandlerInterface;
use Drupal\Core\File\FileSystemInterface;
use Drupal\Core\Form\FormStateInterface;
use Drupal\Core\Render\Element\Checkboxes;
use Drupal\Core\TypedData\TypedDataTrait;
// External libraries.
use GraphQL\Language\AST\DocumentNode;
use GraphQL\Language\AST\InterfaceTypeDefinitionNode;
use GraphQL\Language\AST\TypeDefinitionNode;
use GraphQL\Language\AST\UnionTypeDefinitionNode;
use GraphQL\Language\Parser;
use GraphQL\Type\Schema;
use GraphQL\Utils\BuildSchema;
use GraphQL\Utils\SchemaExtender;
use GraphQL\Utils\SchemaPrinter;
use Symfony\Component\DependencyInjection\ContainerInterface;
Now, to me that's very readable when I need to review or find a use statements - compared to trying to find it alphabetically. I may not even remember the exact fully qualified name, but I know that what I'm looking for is a module or an external library so my eye immediately goes where it needs to go.
Within each group, I think case-insensitive alphabetical sorting makes more sense. For a human, c is the same letter as C. The example in the description of this issue is not particularly useful for a human reader. Fortunately though, this mostly becomes a non-issue with the approach described above because in 99% of the cases modules are grouped separately than core and external libraries, and it's pretty much only Drupal modules that use lowercase.
I have documented this approach here: https://github.com/krystalcode/drupal8-coding-standards
When it comes to PHPCS, I'll come back to my initial point. I frequently get developers updating a file where I have used this method, only to change the import statements back to an unreadable blob with the only objective to make PHPCS go green. We're missing the point here.
That said, I think it wouldn't be difficult to write a Sniff that:
- Allows single empty lines between use statements.
- Allows for comment lines between use statements.
- Recognizes groups of import statements using the empty lines as separators.
- Requires alphabetical sorting - but only within each group.
- Raises a warning if a group contains more than 10 import statements - configurable.
@selfirian the SVB that I currently use might not fit in the specs/aesthetics of the Gin menu; it works for the standard admin toolbar. So my comment was more of a question:
Are there any specs or instructions about the SVGs so that I can get one done for this module?
Thanks for working on this!
Are there any specs or instructions about the SVGs so that I can get one done for this module? The rebuild cache icon is kind of ok, but I use the font-awesome "refresh" for the standard admin toolbar because it has two arrows and the functionality of the module is syncing back-and-forth data between systems, while rebuild cache is in itself.
The Engagement submodule is completely non-functional on branch 2.x. It only works on branch 1.x. The module has been intentionally left as incompatible with Drupal 10. We don't want any new sites to install the submodule.
Moving forward, the submodule should actually be either removed including the entities that have been created. Or, rewrite it if somebody sponsors it, on the same lines that the base module was rewritten.
@shabana.navas @dastan56 I'm not sure if there is a better solution here. If this is to help existing projects to avoid patches or using special git branches, I'd be ok if it is marked as Drupal 10-compatible but change the module's title in the info file to "Commerce Hubspot Engagement [DO NOT INSTALL - UNSUPPORTED]".
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
I have created an MR. Note that in my tests this works for the pages provided by group_term and commerceg_product but not for gnode - the "Add existing content" button redirects back to the listing page instead of sending the user to the add page. That should be troubleshooted before this gets in.
krystalcode → created an issue.
Sorry, I meant to say "reverse entity references".
The most common scenario would be a node referencing a taxonomy term, and you would want the node to be indexed again when the name, for example, of the term changes. The reverse entity reference scenario would be that you would want to reindex the taxonomy term when one of the nodes referencing it change. Why would you want to do that? That depends on the application, there could be plenty of reasons for applications with custom features.
In my case, it has to do with the Group module. You want to index the groups in which entities (nodes, products etc.) belong to so that you can customize the query to fetch only entities that the user is expected to have access to. The entity does not reference the group directly. Instead the group relationship references the entity thus it is a reverse entity reference.
The way the Group module works is a bit of a special case that would justify a custom processor, or a small contrib module - which is what I've done in my case. There is though a processor provided by Search API for normal reverse entity references and I think that should be supported by the module. The main point of creating this issue though is not to cover my specific use case. It is to see if there is a better way to architect a system that would allow custom processors to plug into the existing API i.e. getAffectedItemsForEntityChange() or something similar to determine which entities should be reindexed. Right now this is coupled to forward entity references specifically, I think it could be improved by abstracting certain part of it.
Let me know if that makes sense.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
Field widget removed, replaced by entity reference selection plugin at ✨ Provide entity reference selection plugin for selecting applicable sets Active .
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
- The initial intention to potentially have a content entity as well for logging purge operations as well. This is not supported for now, and it might never be.
- The "bundle" configuration property is a bit different, it is a property used by the Entity module and it has to do with making the PurgeType config entity "bundleable" via the PurgeConfigurator plugin. That's also not needed right now in this module so we're good to remove that as well.
Thanks.
krystalcode → created an issue.
krystalcode → created an issue.
krystalcode → created an issue.
Updated credits.
Updated credits.
@alexpott why would that be the case? Drupal 11 relies on Symfony 7 (https://git.drupalcode.org/project/drupal/-/blob/11.x/core/composer.json...) which also has the same method declaration (https://github.com/symfony/event-dispatcher/blob/7.1/EventSubscriberInte...).
Merged into the main development branch. I will close and make a release after this has been tested on a project for some time to make sure we don't have any regressions.
Notes:
- I changed accessCheck() to accessCheck(TRUE); I prefer to be explicit with access control, it makes it easier to read if you don't remember what is the default.
- Removed a duplicate call to access check, kept the one before the subscriber so that subscribers can change it if they have a legitimate reason.
- I reverted the change in the declaration of public static function getSubscribedEvents() to match that of the interface - see Symfony\Component\EventDispatcher\EventSubscriberInterface.
- Good with removing Drupal 8 support, event dispatching is not compatible anymore.
Thanks!
krystalcode → made their first commit to this issue’s fork.
I have merged this to the main development branch. I set up the tests to run and they seem to pass with Drupal 10. I will close the issue and make a release when this is actually tested on a Drupal 10 site on production for some time.
I have maintained Drupal 8 compatibility. If there's no specific code that would make the module incompatible with earlier versions of Drupal, I don't see the reason to remove compatibility.
Thanks!