🇺🇸United States @paul121

@m.stenta - just want to double check and see if you are still thinking we should add Google Maps support to farmOS core instead of keep it in contrib farm_map_google?

@symbioquine and I chatted about it some on the dev call today. Since the Drupal module side of things should be fairly straightforward (and similar to Mapbox) it doesn't seem like a huge maintenance burden to add to core, so we are generally in favor of that.

paul121 → created an issue.

I've opened a PR on Github implementing these changes: https://github.com/farmOS/farmOS/pull/922

Thanks @m.stenta, fixed! Included in a new 1.0.0-beta5 release.

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

No, a little different. I'm suggesting that our views should require that users have at least 2 permissions. This isn't technically required, but would improve the UX in scenarios when someone only has a single view own land asset permission:

Combined with query_access handler, we could change all of our views to only need the access {entity_type} collection permission. Users could navigate to any of these views but results would be restricted according to their view (any/own) (bundle} {entity_type} permissions. Still an improvement, but also creates some bad UX if you can navigate to an entity view page even if your permissions will restrict from ever seeing any of the results.

Sorry if this issue hadn't been updated in a while (~1 year!), but I will say that my current thinking is aligned with yours @paul121 (I believe).

No worries and sorry if I was extra verbose. I've been thinking this for awhile but wanted to get it written down here, afraid my thoughts were lost in old chats.

I think we can move ahead with the Log module and tag 3.0.0, then change #3413263: Use Entity API query access handler to filter entity queries based on user permissions to "Needs work". Do you agree @paul121?

Yep, makes sense to me!

* We could also add this in the Log module if it makes sense. No strong opinion there.

I suppose it isn't necessary, but don't see why not? I like that Drupal core is providing this and hope it will become more commonly used.

5. Update all core Views to use those permissions.

There might be a little more than just using the collection permissions. I think we need to make sure users still have at least one view operation permission for either any or own entities like I describe above, but this shouldn't be too complex.

Catching up here... just want to mention that I think we are planning to make these changes in a new major 4.0.0 release, in which case making breaking changes (in moderation) should be OK.

Regarding views and permissions:

Adding an access {entity_type} collection collection permission likely isn't a breaking change in itself, but updating views and other interfaces to exclusively use this permission would be a breaking change. Combined with query_access handler, we could change all of our views to only need the access {entity_type} collection permission. Users could navigate to any of these views but results would be restricted according to their view (any/own) (bundle} {entity_type} permissions. Still an improvement, but also creates some bad UX if you can navigate to an entity view page even if your permissions will restrict from ever seeing any of the results.

Solving this UX issue leads me to something that hasn't been mentioned wrt Views and permissions - I think many (if not all) of our entity views should allow access with any one of multiple permissions. For example, the "Animal assets" view should require one of view any asset, view own asset, view any animal asset OR view own animal asset. Our "All assets" view could require either view any asset or view own assetpermissions, but it also seems reasonable to grant access to the all assets view as long as the user has one bundle specific view-operation permission, view any {bundle} asset OR view own {bundle} asset. This complexity is simply not solved by the declarative views config, instead we will need to manually alter access to our view pages as needed (in fact, I believe we already do this for view any {bundle} {entity_type}, just we aren't checking owner permissions?)

So... perhaps deciding if we require an access {entity_type} collection permission on top of these view-operation specific permissions is a smaller issue. IMO we should require the collection permission on views; it's a small addition and the breaking change is negligible, but the main reason being that this permission provides some separation between data model and UX needs. I admit it's not super clear *why* you might want to hide all of our view pages, but this permission would make it easy to do so (and perhaps hide other things that could also depend on the collection permission) without revoking a user's access to actual data.

That said... this more granular access on views is only possible if we add the query_access handler (or something like it)!

Adding the query_access handler to our entity types would certainly be a breaking change. But more than that, I actually think this would be a bug fix.. Right now if a user's role is configured to have the view own animal asset permission, all entity queries (and views, but let's set aside views for now) are not respecting this permission. I would not expect module developers to be considering this edge case and be adding additional checks to their logic - we missed this for many years ourselves! Ultimately, I'm really having a hard time understanding why we shouldn't add the query_access handler.

So one idea might be to provide #1 as an option (and perhaps it is enabled by default), but allow it to be turned off.

I think we should only allow turning off query_access if you also allow turning off bundle + owner permissions, otherwise you introduce potential security issues. Adding this ability to turn this behavior off seems quite complex when the alternative is... just don't use the bundle + owner permissions :-)

Or, if custom Views are being used, they could check the "Disable SQL rewriting" box in the View configuration. I *think* that would allow disabling the query alterations on a View-by-View basis.

There's an additional compromise which is adding whatever we were going to do on #1 to each relevant view as a filter. It is declarative and can be detected easily, without a tag query alter.

It sounds like there is some concern about how query_access and/or bundle + owner permissions would affect these more custom views. Is there any reason these views cannot just depend on higher permissions like view any asset when they want these guarantees? Otherwise there would only be an issue here if you are indeed trying to show a user something that their permissions determine they should not be seeing... now, I agree there might be scenarios where this might be valid, but it really seems like an edge case, is it not? As such I agree that the solution should be disabling SQL rewriting (confirmed in entity module code this should work) - OR maybe not use views, and instead use a custom entity query with access checking disabled (personally I would be more comfortable with this since views can get very hacky in complex situations...)

Add a custom Views argument plugin that calls out to the entity module's methods for adding conditions to queries. Then this argument can be added to individual Views as needed. This would keep things more declarative in the Views YML.

Or, do the same thing in a hook_views_tag_query_alter() and add a specific tag to the Views that need it.

Again, I don't understand why this shouldn't be the default behavior... are there any examples of views in core or farmOS contrib that would not have this enabled?

Does not call $entity->access(), so any additional or PHP-based access logic does not get applied.

Last I just want to mention... if someone is implementing custom PHP-based access logic, they should be aware that this will not be applied in all situations. It is their responsibility to implement additional entity query and views implementations. As explained in hook_entity-access documentation:

Note that this hook is not called for listings (e.g., from entity queries and Views).

Currently, we are choosing to use the entity module permission provider and access handler, but are not using the entity and views query handlers they provide, which is a mistake on our part.

With very custom logic, I agree that option #2 may be the easiest and most realistic (if not only) solution.

This is really a simple change that might not necessarily be seen within the Log module/tests here (maybe not until we upgrade to level 2) but this will be useful for contrib, and notably FarmOS: https://github.com/farmOS/farmOS/pull/906

Note that tests are failing on 2.x branch for other reasons, see here: https://www.drupal.org/project/log/issues/3431729#comment-15942301 📌 Automated Drupal 11 compatibility fixes for log Needs work

I also scheduled the Gitlab pipeline to run nightly.. it hand't run in many months and passed the last time it ran. Hopefully we can notice this sooner going forward!

Opened a MR that fixes the tests to pass on both D10 and D11:
- Run tests on current and previous Drupal major (D10 and D11)
- Update tests to run on Postgres 16 (required for D11, don't think there is an issue with this?)
- Fixed a minor PHPCS error
- Fixed an issue in tests
- Fixed a larger issue in the LogForm constructor. Judging how this crashed the tests, I think this would currently be crashing D11 sites?

paul121 → created an issue.

LGTM - I agree these aren't necessary. I can't remember where that second plugin was used either. Relic of the past.

Thanks @saschaeggi, please see new MR 563!

Hey what is your opinion on the other approach I proposed to generalize the "node edit form" to be the "content edit form"? You can see the changes in that branch here, I just didn't make a PR for it: https://git.drupalcode.org/issue/gin-3342164/-/compare/8.x-3.x...3342164...

It isn't clear to me how much Gin depends on the core claro/node-form library/styles/JS, in which case this could also be a bit of a core issue. Otherwise if Gin really overrides all of the claro node form template + library then perhaps this is an opportunity for Gin to continue further improve this outside of core.

This second condition was not added with 📌 Remove hardcoding #group for the status field Active . It looks like it was added for 🐛 Ensure sticky action buttons to work with modals and ajax refresh-calls Needs review - https://git.drupalcode.org/project/gin/-/commit/a79946204257bd0c9d47fdfe...

I tested this change locally and am not seeing the issues I experienced in 📌 Remove hardcoding #group for the status field Active

maybe we should also open a follow-up issue to optimize \Drupal\gin\GinContentFormHelper::isContentForm as we should probably cache the result of that fairly expensive method.

Oh interesting! I haven't done something like this before. Curious how you would go about that.

Sorry for the above noise. I made a new MR 156 and pushed a few times to get tests passing. This is a slightly different approach at the validation logic so that it checks both the confidential boolean and the secret value. Previously this only checked the secret value.

I tested with the latest 8.x-3.x and confirmed the same issue is happening. There were some changes in GinContentForm so I've rebased my MR 446 onto the latest 8.x-3.x. Again, the full diff for this MR is a little ugly, please review the 4 commits individually to better understand the changes. I changed the order of these commits to make them easier to review.

The first two commits fix the issues previously described in this issue.

The third commit in the MR has the most changes because it combines two if statements into one and un-indents code from a nested if statement to simplify relevant logic in GinContentHelper::formAlter. These are the two if statements before changes:

    // Sticky action buttons.
    if ($this->stickyActionButtons($form, $form_state, $form_id) || $this->isContentForm($form, $form_state, $form_id)) {
      // Action buttons.
      if (isset($form['actions'])) {

In the last commit, instead of calling isContentForm() and invoking `hook_gin_content_for_routes` 3 separate times we can save the result in a variable and reference this value as needed.

Ultimately, the logic in the two above if statements are simplified to:

if (($use_sticky_action_buttons || $is_content_form) && isset($form['actions'])) {

I mentioned in Slack that I might open a separate issue with these additional changes that are not necessary to fix the issue. But because these changes would either conflict with or be dependent on the fix in this MR, it is a much easier contribution to include them here. If maintainers still consider this out of scope then please only consider the first two commits from my MR. Cheers

Checked with installing examples module ( https://www.drupal.org/project/examples → ) also, But not able to reproduce it. Let me know if other specific configuration is needed, thanks!

Hi @maninders, appreciate you trying to recreate this. The important thing is that the form you are testing has a form field called status. I'm not sure if there is a custom form in the Examples module that fits this criteria. Testing likely requires creating a custom form.

I've rebased this MR onto the latest 8.x-3.x, there was only one minor conflict with the last commit with this MR. Still need a review on this

Okay, I've rebased this on 8.x-1.5 and added two new commits to account for the changes in #10 and a relevant change from 🐛 EntityViewsData Column information not available for computed entity reference fields Fixed to use hasCustomStorage()

Including a patch file we can use for farmOS.

paul121 → created an issue.

I've opened a new MR against 11.x. Chose to cherry-pick the existing commits into a new MR because there were unrelated merge conflicts in the core branches between 9.3.x and 11.x. and I didn't want to force-push this old branch. Attaching a patch file for composer builds.

rebuildBundleFieldMap() should finish off by clearing the 'entity_field_map cache item, as otherwise it won't have any apparent effect.

I've added a final step to clear this cache item and updated tests to account for this. One thing I noticed is that these tests are implemented in a new Kernel test file - all of the existing EntityFieldManager tests seem to be unit tests. I'm not sure if that is a blocker for merging.

Without this change, what are the defaults? And what would happen if this were run in a GitLab instance that's different from Drupal.org's? I assume this configuration is tightly tied to that instance?

Yes, as I understand this template is very very tightly configured to the Drupal gitlab instance. I'm not sure what would happen if you ran this on another gitlab instance. From what I understand they have designed the template to "work out of the box" for the majority of drupal contrib modules & to always run on against latest Drupal core and default services. Depending on this template is so much less maintenance work than anything else we can do.

Though I see how database versions could create more work here but I'm not sure what we can do to avoid this. I followed the Drupal Gitlab CI template docs which links to these allowed images. In fact, I had first tried only specifying pgsql-13 and it failed because I wasn't specific enough (13.5 minor version is required). For pgsql 16 and mysql 8 it looks like they are maintaining a single major version of the image that will always use the latest minor release, but they don't have this for mariadb.

The commit that fixes this issue should be called "Issue #3392223: LogActionsTest::testRescheduleMultipleLogsRelative() fails on PostgreSQL" (you can copy it from the "Credit and committing" section down below on this page). That's how we'll know how to find this discussion in the future if we need to from the commit messages.

I just set the merge settings for this log project to use merge commit with semi linear history - is that OK? I think these descriptive commits are more valuable for describing how code is changing rather than just why. Or can we do this when we merge? Worrying about issue #s while working on PRs just makes things more confusing IMO

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

paul121 → created an issue.

We have experienced this same issue in farmOS for some time where the entity status field is not a checkbox. I solved it in other ways (our own version of GinContentForm so to say) but agree with the proposed fix to only move the status when it is a checkbox.

Furthermore, I'm running into this issue on *simple forms* (not entity forms, not gin content forms) after ✨ Move Action buttons to sticky header Fixed . With this change and status field in a form is moved out of the form to the actions area. Not only is this unexpected, but it also results in this status field not being included in the form submission (I believe because the status is moved without the additional template changes that content forms receive?)

I think the fix is two parts:
1. Only move the status field on gin content forms
2. Only move the status field when it is a checkbox

I've opened a new MR with these changes. It also includes some changes to simplify GinContentFormHelper logic and also likely increase performance (don't invoke thegin_content_form_routes hook multiple times!!!)

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

paul121 → created an issue.

paul121 → created an issue.

Oh, and I didn't change the other tests that use similar $timestamp arrays. In 🐛 Update owner of cloned logs to the current user Needs review I've actually refactored how these particular action tests work so they are simpler. But LogActionsTest::testRescheduleMultipleLogsRelative() is particularly complex and can still use this approach, I think :-)

I updated the gitlab CI to run on multiple DBs inlcuding pgsql-13.5 and could reproduce this issue: https://git.drupalcode.org/project/log/-/pipelines/210166

Why is there inconsistency in the order of results returned by loadMultiple() in the first place?

Indeed this is weird. I wonder if it has to do with the order the logs are modified in the tests and postgres uses modified time as a default sort? But loadMultiple() actually returns an associative array with log IDs as the keys, so I'm not sure the order of the key/values in the array is guaranteed.

I've updated the tests so that the $timestamp arrays are keyed by the log IDs. I think this is a more explicit test anyways: we want to guarantee certain logs have certain timestamps, this code isn't responsible for returning logs in any particular order.

I'd like to merge these changes so that we can have tests running on multiple DBs (without failures!) What do you think @mstenta?

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

This is working with Gitlab CI now. See the "test only changes" test run! https://git.drupalcode.org/project/log/-/pipelines/210136

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

saschaeggi → credited paul121 → .

Including patch with latest changes for 6.0.0-beta6

Marking Needs Review since I have refactored some of the logic here, would love a review of this. Acknowledge that we could still use tests

In reviewing this issue again I believe there is another issue regarding client_credentials grant with public clients.

A "confidential" consumer with a field value consumer.confidential === TRUEthat is configured *without* a secret (or an empty secret string) is not validated and could be authenticated. This is a problem. The spec says "the client credentials grant type MUST only be used by confidential clients" - the spec does not care nor know about the concept of a consumer.confidential field, solely that if a client secret is not configured (or has an empty string, per the spec) then client_credentials grant cannot be used.

I've refactored the logic in this validate function to explicitly check for this case with a "public" client + client_credentials grant. I think this makes things simpler and easier to follow. I've added a kernel test to cover this case. Using the Gitlab CI "test only changes" you can see this test fails on current code: https://git.drupalcode.org/issue/simple_oauth-3322325/-/jobs/1539305#L110

I would also want test coverage to ensure that in case the client secret is set to empty the client can not authenticate with a non-empty secret (something I think the current code allows if I look at it). In the case a client provides a secret but is configured not to use one, they're still using an incorrect secret and should be denied.

Good catch @kingdutch. These changes should cover this case as well. With my changes we only return TRUE in one place for a fully correct public client w/o any client secret provided. The final check does a password check only when the stored and provided client secret exist, else returns FALSE.

Unit tests, so that this does not regress again.

Agreed @rudolfbyker, this ClientRepository::validateClient logic would best be covered in a unit test. Unfortunately I don't have much experience writing unit tests and don't have time at the moment.

A way to clear the secret in the UI when editing the consumer. Currently, if you leave the field empty, it just means "don't update".

Agreed this is needed as well. I'm not sure the best approach to do this from the UI. Perhaps we could discuss and agree on an approach before starting work? Personally I'd like to see the UI changes happen in a separate issue. We've been dealing with this issue and patching in FarmOS for a while and would love to just get the validateClient logic in place.

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

Made a new 1.1.1 release with this

That caching makes sense. Thanks for sharing. Will keep this in mind for future cases. Thanks again @wotnak!

Thanks @wotnak! In the past we've solved this issue of hiding local tasks by making the associated route have an access check result/response of 4xx. I've done this in farmOS core and some contrib modules. With your implementation I worry that this might entirely remove the task definition? Unless they are cached for each entity/page? And even if they are, it might just be easier to have a single task that is always visible and use access checking to determine if the task is shown?

.. In fact, it looks like I had already implemented my approach in this module via a Route subscriber: https://git.drupalcode.org/project/farm_project_plan/-/blob/1.x/src/Rout...

BUT _entity_bundles is deprecated and must have been removed from Drupal 10, now we should use bundle https://www.drupal.org/node/3155569 →

I think that would solve it. What do you think of this approach?

Fixed via 📌 Uninstall v1 migrations Fixed

Here is a simple fix. Attaching a patch we can use in farmOS.

Although I feel like there must be a more elegant solution because we are adding various margin-top, only to remove some of it from higher up in the .content-header. But it's tricky because the top margin is added to .help, .region-content and .region-highlighted. https://git.drupalcode.org/project/gin/-/blob/8.x-3.x/styles/base/_layer...

To to properly test a more thorough change I think we would need to consider all combinations of pages with/without local tasks, help text, and status messages, all across different screen sizes.

Attaching screenshot examples

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

I've implemented a first version of this: https://git.drupalcode.org/project/farm_jd/-/commit/eb3785b9c704a7625474...

When checking for new field operations we also enqueue the field operation's shape file to be generated (this is an async operation, can take up to 30 minutes). Most of the time this file should be ready once the field operation is actually imported as a log.

Right now it is only requesting the Point - EachSection shapefile from JD. It also uses the farmOS configured units. For more info see here: https://developer.deere.com/dev-docs/field-operations#/fieldOps/{operationId}/get

Some ideas for future:
- Have a dedicated page that lets you sync shape files "on demand" - this could allow specifying a specific granularity of shape file you want to sync (Point/Polygon, Section, Sensor, Hertz). It could also help remedy issues when a shape file is not able to be synced when the log is created.
- Make auto-syncing of shapefiles entirely configurable. This could also include configuration for the type of shapefile.
- Parse the equipment out from the shapefile. This should be possible, but looks challenging.

Done!

Pretty simple changes. We only use the product vs products when populating Field Operation change lists (so we have a simple name of products/varieties). Update here: https://git.drupalcode.org/project/farm_jd/-/commit/daf671d2400e9b20acd2...

When actually creating logs, we get product information from the Measurements API. I've updated this to use ProductTotal measurements nested under the new ApplicationProductTotal key/list: https://git.drupalcode.org/project/farm_jd/-/commit/74fbc17954ec7717bf1b...

This is specified in the varietyTotals - added here: https://git.drupalcode.org/project/farm_jd/-/commit/daf671d2400e9b20acd2...

paul121 → created an issue.

What if we abstracted BOTH of these hooks (hook_entity_base_field_info() and hook_farm_entity_bundle_field_info()) into a single farmOS EntityFields event, and recommended that modules use that instead of hooks.

This makes sense. I think we would still want separate events to distinguish between base and bundle fields (modules should know which they are providing?) - but yes, we could likely have a single EntityFields event class that could be re-used for both use-cases. And the nice benefit being modules could encapsulate all of this logic into their own, single Event Subscriber class.

Awesome.

I also like referring to this resource: https://gist.github.com/bojanz/c5fcf5cef22406096588

TLDR;

Choosing the right solution:
Are you allowing modules to react to something that has already happened, or alter a structure? Use events. Do you need to allow your class to have multiple instances created by the user? Use plugins. Otherwise, use tagged services.

Most of the time I think we'll want events. But a few of these hooks that are just collecting provided definitions and not altering (like dashboard panes & groups) could potentially use tagged services. A nice write up on them here: https://drupalsun.com/lakshminp/2016/08/17/tagged-services-drupal-8-0

No worries, I'll close that one. I won't have time to work on tests anytime soon.

Attaching a patch we can use in farmOS.

I've opened a new issue in Drupal core: 🐛 Datetime_timestamp widget does not use default field value Active

Thanks for the *fast* response @acbramley! I've opened a new issue 🐛 Datetime_timestamp widget does not use default field value Active

paul121 → created an issue.

Aha. With some help from @wotnak I've found a slightly easier to way to adopt content forms in farmOS. We can register the node_edit_form template ourselves as a little hack with just the following:

/**
 * Implements hook_theme().
 */
function farm_ui_theme_theme($existing, $type, $theme, $path) {
  return [
    'node_edit_form' => [
      'render element' => 'form',
    ],
  ];
}

Once we implement hook_gin_content_form_routes() the content form starts working great.

There is just one issue, this warning is thrown in the log messages the first time you load an entity form after clearing the cache:

User warning: The following theme is missing from the file system: node in Drupal\Core\Extension\ExtensionPathResolver->getPathname() (line 63 of /var/www/html/web/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php)

Deprecated function: dirname(): Passing null to parameter #1 ($path) of type string is deprecated in Drupal\Core\Extension\ExtensionPathResolver->getPath() (line 85 of /var/www/html/web/core/lib/Drupal/Core/Extension/ExtensionPathResolver.php)

This is caused by the implicit (and unnecessary) dependency on the node module as described above. Removing the claro/node-form library solves this issue. I'm attaching a patch for this single change so that we can use it in farmOS, but it should not be considered a complete fix for this issue.

Unfortunately this has caused a breaking change starting with Drupal 10.2. The log entity form no longer sets the default value for the timestamp field and because the field is required, a value must be entered in order to submit the form. If the field was optional then I believe

Previously the datetime_timestamp widget would always use default value set for the field at $items[$delta]->value but now this widget only uses the default value if the parent entity (the log) is not new. This was changed in 🐛 Remove sample date from date field error message and title attribute Fixed : https://git.drupalcode.org/project/drupal/-/commit/5404ebfb11155b53dee0e...

Just re-opening this issue to flag this... we may want to open a new issue in Log or contribute to Drupal core to fix. I left a comment in the relevant issue for now: https://www.drupal.org/project/drupal/issues/2791693#comment-15399165 🐛 Remove sample date from date field error message and title attribute Fixed

I believe an unintended change was introduced with this issue. If a timestamp field provided a default value or default value callback, that field-level default value is no longer used as the default value in the entity form when creating a new entity. There is a check in TimestampDatetimeWidget::formElement() to only use the field item delta value if the entity is "not new": https://git.drupalcode.org/project/drupal/-/commit/5404ebfb11155b53dee0e...

A timestamp field with a default value might be a common use-case when the field is required (the user must provide a value) but the application would like to preset a default value to the current time for convenience. For an example see 📌 Make timestamp required Fixed

paul121 → created an issue.

paul121 → created an issue.

LGTM

And a re-roll for 10.2.x. There was a change to the TaxonomyTermPagerTest modules 8d2f88bf since 10.2.0 that conflicts. Having trouble generating the interdiff for this.

Re-roll of the patch in #80 for 10.2.0. The TokenReplaceTest file has been removed.

hestenet → credited paul121 → .

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

paul121 → created an issue.

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