Not sure if this is the right place for this (please point me in the right direction if there is a more useful place), but just thought I'd drop in a few things that I'm struggling with while developing an API over the last few weeks. These mainly revolving around the extensibility of core JSON:API services due to method access modifiers (predominantly the EntityResource controller class).
What lead me here is I wanted to try and re-use the core code in EntityResource::getJsonApiParams() and EntityResource::getCollectionQuery() in a Custom Resource by calling these methods, but couldn't because they have protected access, I then went down a rabbit hole...
For context I'm creating a few Custom endpoints using the JSON:API Resource → module with JSON:API Extras → installed to enable me to easily alias resources and provide defaults, and JSON:API Include → allowing me to provide a cleaner response for the consumer in certain scenarios. Quite the module cocktail!
All of these modules are great and have worked OOB. However a lot of the time they have needed to modify a core JSON:API service, add a "shim" service or create a new service that extend an existing one etc. The main reason for this is to allow them to extend protected methods on the base service. Trying to build on-top of this very tricky when developing a module to extend the core features as you now have to be aware that popular modules (I imagine JSON:API Extras is pretty much a standard) are rolling their own instances of core services so you cannot reliably extend them yourself.
For example jsonapi_defaults part of JSON:API Extras replaces the class used for the jsonapi.entity_resource allowing them to add default parameters in the protected getJsonApiParams method. Had the getJsonApiParams been public the same could have been achieved with a service decorator, which would then allow other modules to add decorators as well without having to know what other modules are trying to extend these services. As it stands, if I want to extend some functionality to `getJsonApiParams` and retain the functionality provided by jsonapi_defaults, I have to include the jsonapi_defaults module as a dependency and extend that class.
As the module ecosystem around JSON:API grows, this is going to a bit of a blocker. Apologies if I don't have the back-story to the method access on these services (I'm sure there is a good reason)!
My two cents (for what it's worth) on things that might help the DX around extending JSON:API:
- Merge functionality offered by JSON:API Extras into core?
- Open up method access on core services. Allowing useful things like:
$params = \Drupal::service(Routes::CONTROLLER_SERVICE_NAME)->getJsonApiParams(Request $request, $resource_type);$query = \Drupal::service(Routes::CONTROLLER_SERVICE_NAME)->getCollectionQuery($resource_type, $params, $cacheability);
- EntityResource controller is doing too much IMO
- Filter parsing logic could be broken out into a separate public service? This could then be easily extended by decorators.
- Query building logic could be broken out into a separate public service? This could then be easily extended by decorators.
- Response building logic could be broken out into a separate public service? This could then be easily extended by decorators.
- Triggering some events during the execution of all of the above steps might also be an easier way to open it up slightly?
Hopefully that's a useful view of working with the module and it's ecosystem (which is ace btw) and might help a bit with moving it forward. For now I'll build my new features specifically for the application in question as I know what the dependency tree is and can extend the relevant classes.
Cheers! 🤟
Just going to add this here for anyone landing here wanting to do this for all address_country form elements by default without having to write/configure custom FormElements/FormWidgets.
/**
* Implements hook_element_info_alter()
*/
function mymodule_element_info_alter(array &$info) {
if (isset($info['address_country'])) {
// Add a process callback (after the defaults) for address_country form elements
$info['address_country']['#process'][] = 'mymodule_process_address_country';
}
}
/**
* Process callback for all address_country form elements
*
* @param array $element
* @param \Drupal\Core\Form\FormStateInterface $form_state
*
* @return array
*/
function mymodule_process_address_country(array &$element, FormStateInterface $form_state) {
$element['country_code']['#type'] = 'select2';
return $element;
}
This will change the address_country element everywhere it is used on the site(even within the full address widget). If you want to be able to pick and choose where select2 is used, use one of the widget examples above!
Cheers!
Hey Nitesh Sethia,
It looks to me like you have bought the broken changes from #6 (Invalid hook arguments etc, see full list in #9) into the MR.
The work I added to the MR was to address some of the issue raised (specifically arround hook parameteres) in #9. This work was intended as a replacement for the work done in #6. It looks like you've added the work from #6 back?
(Sorry I should have made that clearer when I added the changes)
Cheers!
Same issue runing v3.4 on Drupal 10.1.2. Patch in #9 fixes the issue. Thanks!
I've opened a MR with my suggested changes. Feedback welcome!
MR15 will be a breaking change for anyone using a serialized mutivalue key in it's entirety as a config override (Not sure if this would actually work in reality, but it can currently be configure that way).
Unfortuantly there is no easy way to address this. I did explore adding a new field to the KeyConfigOverride and only unseriaziling if it was set, but by the time the override value is actually set in KeyConfigOverride::loadOverrides we have lost context of which KeyConfigOverride entity supplied to override, meaing we can't use any of it's properties to influence the setting of the value.
Incidently this also means that the suggested approach of mapping to a specific field on a multivalue key will also not work.
The only way that I can see around this is to change the KeyConfigOverride::getMapping method to store the KeyConfigOverride id rather than the Key ID and then retrieve the key id (and any associated config we need) in KeyConfigOverride::loadOverrides. But that would probably result in all existing KeyConfigOverride breaking, very much a bigger change.
Ran into a similar issue when storing user login credentials. I used a `user_password` KeyType to store the key data and needed to override a `credentials => ['username' => '', 'password' => '']` config structure with the key.
Decided to go a slightly different route in MR15 than that suggested approach, and just allow whole sections of config to be overriden by a key if needed rather than picking out individual values.
Hey Anybody,
This is still an issue on 2.0.2 and the patch in #23 still fixes it for me.
To recreate, set the outtermost DIV of a Infinite Scroll View to have CSS properties of overflow-y: scroll and a fixed height, when the bottom of the DIV is reached the load in not triggered. There is a test included in the patches.
Cheers!
For those landing here and wondering what a custom widget to get around this might look like, this kind of thing seems to do the trick:
<?php
namespace Drupal\my_module\Plugin\Field\FieldWidget;
use Drupal\Core\Datetime\Plugin\Field\FieldWidget\TimestampDatetimeWidget;
use Drupal\Core\Form\FormStateInterface;
/**
* Plugin implementation of the 'nullable datetime timestamp' widget.
*
* @FieldWidget(
* id = "datetime_timestamp_nullable",
* label = @Translation("Nullable Datetime Timestamp"),
* field_types = {
* "timestamp",
* }
* )
*/
class NullableTimestampDatetimeWidget extends TimestampDatetimeWidget {
public function massageFormValues(array $values, array $form, FormStateInterface $form_state) {
// Record any NULL values
$null_values = [];
foreach ($values as $delta => $item) {
if (is_null($item['value'])) {
$null_values[$delta] = $delta;
}
}
// Let the core widget do what it wants
$values = parent::massageFormValues($values, $form, $form_state);
// Restore the null values
foreach ($null_values as $delta) {
$values[$delta]['value'] = NULL;
}
return $values;
}
}
Nice work guys! 👍
Can I suggest that if we're talking about a new 2.x branch that it might be worth rolling this and the following issue in together:
https://www.drupal.org/project/field_permissions/issues/3110867 →
The patch on that issue already uses patch #10 from this issue as a base.
+1 for the patch in #9.
Managed to successfully use the patch to acheive the above use case. I used an OR filter group containing a = FALSE condition and a IS NULL condition. Worked perfectly, thanks!
* Other option, create a View with an optional relationship to something that has a boolean field. In the rows that don't have the relationship the values will be NULL
As Lendude mentions here this is a massive gap in the current boolean filter logic, take the following scenario:
I have a View that lists multiple bundles of the same entity type, one of those bundles has a boolean field. I want to be able to filter the view to only show records that don't have that field (all but one of the bundles), or have a false value for the field. This is currently unachievable.
To my mind this is the main use case we are solving here.
For those landing here looking for a way to add #states to field_groups in a more generic way via traditional form_alter_hooks, the below helper hook works well.
/**
* Implements hook_field_group_form_process().
*/
function my_module_field_group_form_process(array &$element, &$group, &$complete_form) {
/**
* Provides a generic way for assigning Form API #states to field group elements
*
* @usage - In a from_alter hook the Group object can be added to include a form_api_states property,
* which is picked up here and applied to the field group element
* @code
* $form['#fieldgroups']['group_my_group']->form_api_states = [
* 'visible' => [
* ':input[name^="my_field_name"]' => ['checked' => TRUE],
* ]
* ];
* @endcode
*/
if (isset($group->form_api_states) && is_array($group->form_api_states) && !isset($element['#states'])) {
$element['#states'] = $group->form_api_states;
}
}
Nice work Jan-E, thanks for improving that code and patchifiying it.
+ if (!$logged) {
+ \Drupal::logger('pipelyft_subgroup')->debug($this->t('Subgroup lock hit for parent: %id - %label. Child: %child_id - %child_label', [
+ '%id' => $parent->id(),
+ '%label' => $parent->label(),
+ '%child_id' => $child->id(),
+ '%child_label' => $child->label(),
+ ]));
+ $logged = TRUE;
+ }Just a quick note, you might what remove this bit as it's got specific logging references to the project that I'm working on. Apologies I must have missed that when I posted the initial code.
The point is that reverting to an earlier revision also reverted the left/right values to those of the earlier revision. That is why we have to update all revisions with the new left/right values.
Right yea makes sense, because the new leaf will still exist even if you are reverting the current Group to an older version.
What I do know is that a group is already deleted from the database before we reach doRemoveLeaf. Which means that we have got a corrupted tree when doRemoveLeaf is not executed somehow.
Yea this is why I reverted to using the wrapLeaf method when deleting Groups because the tree values at that point are no longer in the database, but are still stroed in momory on the Group object so can be accessed that way. Is the below not working for you then?
// We have to use the values stored on the Entity object rather than going to the database when deleting groups
// because by this point in the delete transaction the field values have already been removed from the database
// The easiest way to do this is to use the leaf wrapper.
$leaf = $this->wrapLeaf($entity);
Did you already have a chance to test the latest patch in your situation, without revisions? I have put that patch in production last night, with some additional logging. Initial experiences are OK. But the cache-out-of-sync cases happened only every couple of weeks. That piece of code is still there, so if it happens it will be logged.
I have not tried it yet, the lead time on changes like that my end are quite involved. It looks like it would work fine though from taking a look at it. I will try to test it locally when I get round to looking at it again, but unfortuantly my focus is on other things at the moment!
querying for the ids_to_update and afterwards building a query with a condition if the entity_id is IN the ids_to_update array seems a bit redundant. That can probably be done in one query
Yea my thinking behind doing this first was that it would prevent any potential issues with updating the wrong values if the left/right values had changed in the time that we ran the two queries. I.e in that current setup both queries know exactly what they need to update, regardless of the left/right values. However your probably right the chances of that happening are slim to none I would have thought.
Cheers!
Hey @Jan-E, I'm gald you found the code useful!
Yea apologies, the revisions were a bit of an afterthought I must admit as we don't use revisions for groups on the project this work was for, so only focused on what we needed.
Glad to see that you have managed to retro fit them! Revision may be a little challenging with this approach because we are bypassing the standard revision creation logic by updating the tree values directly in the database. Has the new revision already created by the time we get to this code? If so I'm surprised that just updating the latest revision wasn't enough.