Refactored in the MR:
Drupal\commerce_cart\Controller\CartControllerDrupal\commerce_cart\Controller\CartBlock
Other places that could follow the same:
Drupal\commerce_cart\Context\CartCacheContextDrupal\commerce_cart\EventSubscriber\QueryAccessSubscribercommerce_cart_user_login()- this might be easier to customize if moved to a hook service
Re-uploading patch from #5 with some changes removed because they were moved to a separate issue.
krystalcode → created an issue.
Moving to Needs Review to get some initial feedback from module maintainers.
Remaining tasks as of now.
- Get feedback from maintainers to ensure they're in agreement with the approach.
- Add
NULLhandling and deprecation notice for the extra argument in the constructor.
Some more comments.
- I'm more in favor of unit tests, but since I know other like kernel or functional tests more those could be added/updated.
- Unit test are added this way so that classes extending the cart provider, for example, to alter the query, can easily add their own unit tests so that all checks are run with the changes without having to repeat all this code. Not sure if there's a better way.
- The tests could be refactored to reduce code repetition, but I think it would make code less understandable. For tests, I think there should be quick visibility on what exactly the test checks for without having to go through levels of inheritance and other tricks.
- We do lose the ability or add or remove the cache item for a specific cart when created or finalized - we have to clear all caches for the user. But I think that's more than compensated by the other performance advantages e.g. not loading cart entities at all for authenticated users - unless requested, limiting the number of carts requested and loaded etc.
Adding a patch for Commerce 2.33, I'll create an MR against the latest 2.x version soon.
krystalcode → created an issue. See original summary → .
krystalcode → created an issue.
This may be a matter of preference or semantics. The same way that within Drupal we don't define internal redirects using the full URL including the scheme/hostname etc., we just use the path, similarly on the client side - for example on Next server - you define a redirect like NextResponse.redirect(new URL('/login', request.url)). It does not make much sense to say NextResponse.redirect(new URL('https://www.example.com/login', request.url)) if you are already on that domain. That said, passing the full URL would work as well. I don't know Next well enough to say whether internally there's any functional or performance-related difference.
Semantically - together with the isRedirect property, this basically says that "you're staying on the same site but you're redirected to another page". Otherwise, if the frontend just wanted the final internal path it would either have to extract it from the resolved property (not that it's a big deal), or from the last item of the redirect array which is unnecessary code for the frontend to handle. The former option i.e. resolved property actually - and if I remember correctly - would not properly carry over query parameters and fragments, while - together with some other fixes on this module - they would be carried over in the redirect array.
Together with some other changes to carry over query parameters and fragments in the resolved that I have not added here on this module, all that the frontend has to do is to grab that path. For the bigger context that may be useful when reviewing other issues that I opened in this module, here is what we were trying to achieve.
This came up as part of a bigger effort to make things as simple as possible for the frontend. We found that a decoupled frontend would have to write quite a bit of code to handle different cases e.g. path given as a URL alias, path given as the internal path, path or URL alias that gets redirected, redirects to internal vs to external URLs, issues with carrying over query parameters from the given path and from any applicable redirects, some special handling for the home page that can be found as / or as internal path or as URL alias of the node etc.
To have logic for handling all these different cases in the frontend ended up being time consuming and error prone i.e. all sorts of bugs were found, fixing something only to break something else etc.
At the end, here is the final structure that we ended up with. We found that this structure minimizes the logic that has to be done in the frontend on all cases. Example response:
{
"resolved": "/my-content-url-alias?query=parameter",
"isRedirect": true,
"isExternal": false,
"isHomePath": false,
"entity": {
"canonical": "http://www.example.com/my-content-url",
"type": "node",
"bundle": "page",
"id": "1234",
"uuid": "73c03483-XXXY-YYYY-d9acbdf4d5c0",
"path": "/my-content-url"
},
"label": "My content item",
"jsonapi": {
"individual": "http://www.example.com/jsonapi/node/page/73c03483-XXXY-YYYY-d9acbdf4d5c0",
"resourceName": "node--page",
"pathPrefix": "jsonapi",
"basePath": "/jsonapi",
"entryPoint": "http://www.example.com/jsonapi"
},
"meta": {
"deprecated": {
"jsonapi.pathPrefix": "This property has been deprecated and will be removed in the next version of Decoupled Router. Use basePath instead."
}
},
"redirect": [
{
"from": "/node/5678?v=1",
"to": "/my-content-url-alias?v=1",
"status": "301"
}
]
}
In most cases we do not want to always expand the hierarchy; that can make large tress less user friendly. Instead, only expand what is needed to traverse the tree up and down as needed for furthering your filtering i.e. expand children when the result is active or has active children.
There is a then setting "Always expand hierarchy" that can be enabled if you do want the hierarchy expanded regardless for your case.
The problem is that the facet widget implementation in this module does not take into account the above. I have created a new MR fixing these.
krystalcode → made their first commit to this issue’s fork.
I'd love to actually improve things at the source instead of implementing somewhat bogus changes just to make phpstan happy
I agree about improving this at source. I did not come across this change just from PHPStan. I wanted to extend form validation by extending the class rather than via a hook, and I wanted to have access to the built entity with the attached values. I finally ended up writing a validation constraint so I didn't use it, but I still think it's proper for the form to behave as expected by its parent.
@alexpott
A couple of comments in the MR. In the original patch I used the isEmpty() method because it is the API method that takes into account how the field type defines "empty". For this field type, here is the definition of empty.
https://git.drupalcode.org/project/redirect/-/blob/8.x-1.x/src/Plugin/Fi...
Real world applications are messy and you can end up with unexpected values due to bugs. Based on this definition, a value like ['path' => '', 'query' => []] should be ignored and the field should be considered as empty. Not sure how other getters handle these, but as an API method I think it's cleaner for the caller to return [] in such cases, and the full value can be retrieved by get('redirect_source') if needed. It would be a bit inconsistent if the field item list would tell you that the field is empty while the getter would give you a value.
Also, the null-safe operator (?->) was introduced in PHP 8.0. this module does not define its compatibility neither with Drupal core nor with PHP in composer.json, but it is declared as compatible with Drupal 9.2+ in its info file which is compatible with PHP 7.3 and 7.4. This makes the module installable in settings where this would cause fatal error.
If the module has reached the point where maintainers only wan to support PHP 8.0+ or Drupal 10.0+, then it should be declared accordingly. But I don't think that should be done just for this issue, I'm personally a bit more conservative and trying to not break other people's websites.
@rajesh.vishwakarma @alexpott
I do not recall the exact steps to reproduce this. But more or less I believe it was something like the following. I was working on extending the form validation at RedirectForm::validateForm(). The parent method from ContentEntityForm would build the entity, and if the source had an invalid value submitted (when creating new redirects), then I would call $entity->getSource() and come across this error.
This was while developing and it may be the case that you wouldn't come across this in normal circumstances. However, I do think that it would still be good to handle this gracefully.
I'm not sure if there is a more general convention in Drupal core/community regarding the level at which a required field should be enforced. You first create the entity at the storage level i.e. $storage->create([ ... some field values ... ]). The bundle (if the entity is bundleable) is enforced there. I believe I have seen some entity reference fields required at that level for some entities.
Other than that, in general, entities are still created and usable with missing required fields. I think that's correct because you may need to do calculations to set the values before you reach the point of being able to set the values for all fields. Then the caller should ensure that - when needed - entity validation is called before saving the entity and that would catch these errors.
If module maintainers want to enforce this value to never be empty from the beginning of the entity's lifecycle, I think this should be happening at the $storage->create() level - then the getter may be left to throw an error. Otherwise the getter should handle this gracefully.
krystalcode → created an issue.
Thank you for your question.
I am aware of the External Entities module and let me clarify that I have not used it yet. My understanding may therefore be incomplete or even wrong. That said, from the module description and its code, it seems that it covers a quite different primary use case.
Entity Synchronization
You have the Commerce Product module installed. It provides a Product and a Variation entity already which have a functional purpose in the application - the commerce website is the Drupal application. You have an external ERP system where product editors manage the catalog; they add new SKUs, update prices etc. You want to bring such product data, like SKUs, prices, titles, descriptions, images etc. and store them as new or in existing products/variations so that the Drupal application is automatically updated. Entity Synchronization will bring in the data and store them into entity fields of the existing Product entity type.
Customers then place orders using your Drupal application. You want to export these orders and its line items to the ERP so that they can be moved to fulfillment. Entity Synchronization provides the framework for automating these exports.
Entity Synchronization does not provide an entity type for you. You bring in the data to whichever entity type you like.
External Entities
You have one or more external systems that provide certain entities, such as content or products. Maybe you run a Shopify store and a Wordpress blog/article publishing site. You want to use Drupal for its editing capabilities and as a central editing hub so that you can create and update those entities in one place. This way you don't have to go to different systems and edit items in different places.
External Entities provides an "ExternalEntity" entity type for use in all these external entities so that you can bring them into Drupal. They are unlikely to serve a functional purpose in your Drupal application, you just want to edit them in Drupal.
Another use case seems to be to bring in content from external sources and essentially use Drupal as a content aggregator. The content may then be displayed to end users or not. It depends on the exact use case, but I wouldn't use the External Entity entity type for that. I'd use the Node entity type provided by Drupal core and use Entity Synchronization to bring in the content.
Summary
There's lots of architectural differences and differences in the supported features, and that's a result of the two modules having a different primary objective; I won't get into analyzing all of them. I'd say that Entity Synchronization is by its nature more generic and flexible; all cases covered by External Entities can be covered by Entity Synchronization. So, if you are looking exactly for the use case that External Entities is designed for, use that. If your use case is not covered, or if you have multiple integrations some of them covered and some not and you don't won't to have to learn and use two different systems, then use Entity Synchronization.
Let me know if you have further questions, reopen ticket.
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.
I think that it is important to keep the change in getEnabledFacets for performance reasons. Facets with status: false should be filtered out. If you look at the initFacets, the manager still not only loops over all facets but it actually instantiates all of its processors and even runs its pre-query processing - this looks like a bug. For Drupal core, if you have the "status" in the entity keys - which is the case here, then that config entity is considered to be disabled.
On complex, multi-domain or multi-site applications, there can be hundreds of facets as stated in the issue description.
If there are issues elsewhere related to config e.g. blocks etc., I think it is best to handle them in different ways e.g. config splits, domain config overrides etc.
I think this needs a bit more work to make sure we don't harm efficiency for the various different use cases.
In my case I display hierarchy by having "Use hierarchy" enable and also "Show parents" from ✨ Add processor to show parents in hierarchical facets Active .
The Taxonomy hierarchy plugin already loads the parents and caches them statically. This MR would result in - in many normal use cases - potentially hundreds of database queries again that trigger access hooks as well, because it loads all ancestors for each term without using the cached items from the hierarchy plugin.
Maybe the best in many cases - from the point of view of reducing database queries - would be to load the entire tree (which is just one database query) in a processor plugin and make it available somehow to the sorting plugin where it can find the parents algorithmically. This may need some refactoring in the manager anyways.
krystalcode → created an issue.
Module maintainers feel free to alter the hook ID, I just used the same as with the cache ID with the _info suffix.
krystalcode → created an issue.
@solideogloria each to its preference, that's why PHPCS rules are customizable. My objection is to make one or the other as a MUST in the standards.
Just to point out the following in your comment though.
It makes more sense for Core use statements to come first, because Core code is the foundation, and the modules build on top of that. Plus, a single list of alphabetically ordered statements is the easiest to sort through visually
The first sentence is exactly what I'm proposing i.e. logical grouping of import statements, whether core or modules come first that's secondary.
The second sentence is the opposite i.e. alphabetical sorting without logical grouping. Drupal/Core is not the first set of packages alphabetically, it just happens to be so in the examples that I listed.
krystalcode → created an issue.
Here is a first take on this - moving the issue to review so that the module maintainers can respond on the approach before taking this further.
Having try to make some customizations and fix a number of bugs, I have found that the code could be better structured and make things more easily alterable/extensible. I have therefore started moving some functionality related to path processing and route matching to separate classes. The event class and router translator should be later updated to not have to compute the same things twice when no redirects are involved.
This patch is incomplete and while I have tested it for my use case (together a number of other patches and bug fixes/customizations) it might introduce bugs for other applications - so, don't use it yet. It's just a starting point to see if that direction works for others or not.
krystalcode → created an issue.
Changes:
- The return as pointed out in #7.
- Added isExternal, I think it's useful for clients.
- Added isHomePage as well for consistency.
- Minor optimization when creating the path.
- Setting the status code when ResourceNotFoundException occurs is not needed, it's already 404 set in PathTranslatorEvent
Adding a few thoughts here. If I understand this issue correctly you want to be able to add bundle fields via the UI.
What you should ideally be doing is adding base fields to the entity programmatically. Adding new fields as "bundle fields" i.e. field and storage definitions in configuration files would harm performance since they are always created as separate tables in the database forcing extra joins in the database queries - and redirects (potentially multiple in redirect chains) may be loaded in every request.
Adding new base fields is really straightforward, including displaying them in forms. Contrib modules can do that for common use cases e.g. a redirect_comment module could provide a comment field for redirects and a redirect_category module could provide a field referencing a taxonomy term for classification.
That said, if a UI and the ability to add bundle fields were to be added, and since entity type definitions are cached, I do not think there will be too much of a performance hit for applications that do not use bundle fields - just a little bit. I would be more concerned about promoting the non-optimal way, and also adding more code to maintain to the main module. It might be best that such ability goes to a separate module or submodule that could alter the entity type and make it fieldable, and provide some notes that this should be used by site builders while developers should prefer base fields where possible.
Missing import statement.
This is a patch as a starting point; you won't get the redirect trace with this. Other than that works fine for me and solves various issues such as support for redirects with query parameters.
krystalcode → created an issue.
Implementing the quick - but less efficient - solution for now.
krystalcode → created an issue.
krystalcode → created an issue.