Looks good. The failing tests are because of upgraded PHPUnit, so they need fixing separately.
I still don't understand how this differs from the form that Group automatically makes like at for example `/group/1/content/create/group_node_request%3Aarticle` and `group/1/content/add/group_node_request%3Aarticle`. But if (re)using these forms doesn't do what you need. If you could open a new feature request issue, and describe what is needed that is different from those forms I think that would help.
I've removed that form. If it has value we should remake it so that it asks which group to add the entity to - I think?
I found more grequest overlaps so fixed them at the same time, and added a couple of tests.
I'm not 100% that they've all be cleaned up. So I'll roll out another alpha.
If you spot some more do open another issue, MR.
It helps, me at least, if you keep the MR with multiple individual commits, for each issue/problem/thing-being-fixed, rather than as one bigger commit.
The autoloader not being updated is, from my understanding causing quite some confusion when geocoder plugins, which are not modules, and are installed only with composer, are sometimes not detected. 🐛 How to add Gecoder 3.x providers Needs review
The issue I described in comment #33 https://www.drupal.org/project/geocoder/issues/3153678#comment-14203727 🐛 How to add Gecoder 3.x providers Needs review where you install the plugin using composer, but the autoloader is not rebuilt, so it's not detected, should I think be fixed with:
📌 Use a better container cache key Active
I'll close this in preference to above.
Potential solution here: 📌 Use a better container cache key Active
I'm wondering about the form group-request-node. I notice I'd made a comment in the test that the module should probably use the default group generated forms.
Did you just have the error when the module presently called this form.
Or is there a good use for using a different form?
Hi,
Thanks for this, it's quite likely there are issues with grequest (as lots of the code came from there). I'll try and get to test it soon. One thing I noticed immediately though. Could you not use {$plugin}->getBaseId() in place of getPluginId() and then looking for the str_starts_with?
> The issue is that the entity browsers ignore this setting, as they are just a view. We have found (due to our use of many geo entity types)
> that we have had to create separate entity browsers to avoid this.
> Wonder if its possible to send something from the field configuration to the entity browser view to apply a filter.
https://github.com/localgovdrupal/localgov_events/issues/186
Removing the canonical link template, and changing the line in module_builder_entity_type_build() has also so far worked.
FWIW I just had a quick look at media entity (which also sometimes has canonical the same path as edit-form) it does still have a view builder defined in the handlers... ah that's for the case where it does have a standalone url. In the other cases the route_provider class returns the edit form route for the canonical route: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/media...
Which module is that? Could be. It's a council's website.
I just installed Module Builder, to generate, a new module and a couple of plugins, but this time I'm not getting past submitting the new module from. Removing the link template, the form submits, but then just leads me to a whole other set of errors however.
I'm getting this on a new module at the moment.
I note there is a canonical link template:
https://git.drupalcode.org/project/module_builder/-/blob/4.0.x/src/Entit...
But there isn't a view_builder handler:
https://git.drupalcode.org/project/module_builder/-/blob/4.0.x/src/Entit...
Both of which are required to build the route in the inherited route_provider.
Should be fixed now.
Not quite. Now it's kicked the Drupal Tests back into action phpunit is failing.
Thanks for the review. Looks good. LGTM.
finn lewis → credited ekes → .
markconroy → credited ekes → .
finn lewis → credited ekes → .
finn lewis → credited ekes → .
ekes → created an issue. See original summary → .
This is still valid after
🐛
User 1 isn't the super user anymore in Drupal >= 10.3
Active
. Although it needs to use $this->getAdministator() to get the correct uid to insert.
The issue was, and still is, that if the UUID of the required owner field of an exported entity is not in the importing site the the value of the owner field is discarded, the required field then has NULL - hence failing to import.
I use "owner field" here. It's usually uid but I'm not sure it has to be does it? It is however maybe the most common case of a required field referencing a uuid that might not exist in the system that can reasonably be reassigned to another (default admin user) reference.
finn lewis → credited ekes → .
finn lewis → credited ekes → .
> I took that using constructor property promotion should be encouraged for new code
+1 for that interpretation. We did already make that assumption in LGD code; plus imho it's soooooo much nicer.
Related 🐛 Date augment, remove occurrence Active
So in my digging into how these things can be done. I've logged what happens with the ical feeds of a Google calendar. Overriding, changing, and removing. Figure I'll log it here too as it might be useful.
https://gitlab.com/ekes/2025-date/-/blob/notes/ical-rrule-override-seque...
Referencing the RECURRENCE-ID works here, as it references the time in the sequence it would have happened, no matter other individual alterations, including removal. Google does not try and reconcile overrides if you change the underlying sequence. Although interestingly it seems they might remain orphaned with no data, as if the underlying sequence returns to the same start times, the empty overrides reappear in the feed.
finn lewis → credited ekes → .
ekes → created an issue.
Reviewed and spotted one last nullable type. For the rest looks good.
Note this does not change the function signature see my note about this on https://github.com/commerceguys/addressing/pull/221#issuecomment-2582855457 so shouldn't require more than a patch release.
Will require https://www.drupal.org/project/date_recur/issues/3498936 🐛 DateTime Range Views Data was converted to a class, hook removed Active
Passes PHPUnit.
I don't thing the cspell, phpstan, phpcs are related to this change?
Was just coming here to do something just like this. Patch adds nullable to all declarations that have a NULL default, and shuts up our 8.4 test errors \o/
I'm just trying to recreate this. Are there some steps I could follow?
Shall open a new issue for any remaining/new.
\Drupal\Core\Render\Element\FormElement::* is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use \Drupal\Core\Render\Element\FormElementBase::* instead. is address module.
The "Drupal\entity_browser\Entity\EntityBrowser::*" method will require a new "string *" argument in the next major version of its interface "Drupal\entity_browser\EntityBrowserInterface", not defining it is deprecated. is Entity Browser module.
Deprecation we should probably catch
1) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
The "Drupal\entity_browser\Entity\EntityBrowser::setWidgetSelector()" method will require a new "string $display" argument in the next major version of its interface "Drupal\entity_browser\EntityBrowserInterface", not defining it is deprecated.
Triggered by:
* Drupal\Tests\geo_entity\Functional\GeoBundleCreationTest::testMediaTypeCreationForm
/var/www/html/web/modules/contrib/geo_entity/tests/src/Functional/GeoBundleCreationTest.php:74
* Drupal\Tests\geo_entity\Functional\GeoOverviewAccessTest::testOverviewPageAccess
/var/www/html/web/modules/contrib/geo_entity/tests/src/Functional/GeoOverviewAccessTest.php:106
* Drupal\Tests\geo_entity_address\Functional\AddressFormsTest::testCrud
/var/www/html/web/modules/contrib/geo_entity/modules/geo_entity_address/tests/Functional/AddressFormsTest.php:86
2) /var/www/html/vendor/symfony/error-handler/DebugClassLoader.php:341
The "Drupal\entity_browser\Entity\EntityBrowser::setSelectionDisplay()" method will require a new "string $display" argument in the next major version of its interface "Drupal\entity_browser\EntityBrowserInterface", not defining it is deprecated.
Triggered by:
* Drupal\Tests\geo_entity\Functional\GeoBundleCreationTest::testMediaTypeCreationForm
/var/www/html/web/modules/contrib/geo_entity/tests/src/Functional/GeoBundleCreationTest.php:74
* Drupal\Tests\geo_entity\Functional\GeoOverviewAccessTest::testOverviewPageAccess
/var/www/html/web/modules/contrib/geo_entity/tests/src/Functional/GeoOverviewAccessTest.php:106
* Drupal\Tests\geo_entity_address\Functional\AddressFormsTest::testCrud
/var/www/html/web/modules/contrib/geo_entity/modules/geo_entity_address/tests/Functional/AddressFormsTest.php:86
3) /var/www/html/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
\Drupal\Core\Render\Element\FormElement::processGroup() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use \Drupal\Core\Render\Element\FormElementBase::processGroup() instead. See https://www.drupal.org/node/3436275
Triggered by:
* Drupal\Tests\geo_entity_address\Functional\AddressFormsTest::testCrud (2 times)
/var/www/html/web/modules/contrib/geo_entity/modules/geo_entity_address/tests/Functional/AddressFormsTest.php:86
4) /var/www/html/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
\Drupal\Core\Render\Element\FormElement::valueCallback() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use \Drupal\Core\Render\Element\FormElementBase::valueCallback() instead. See https://www.drupal.org/node/3436275
Triggered by:
* Drupal\Tests\geo_entity_address\Functional\AddressFormsTest::testCrud (2 times)
/var/www/html/web/modules/contrib/geo_entity/modules/geo_entity_address/tests/Functional/AddressFormsTest.php:86
5) /var/www/html/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
\Drupal\Core\Render\Element\FormElement::preRenderGroup() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Use \Drupal\Core\Render\Element\FormElementBase::preRenderGroup() instead. See https://www.drupal.org/node/3436275
Triggered by:
* Drupal\Tests\geo_entity_address\Functional\AddressFormsTest::testCrud
/var/www/html/web/modules/contrib/geo_entity/modules/geo_entity_address/tests/Functional/AddressFormsTest.php:86
6) /var/www/html/web/core/lib/Drupal/Core/Test/HttpClientMiddleware/TestHttpClientMiddleware.php:52
Renderer::renderPlain() is deprecated in drupal:10.3.0 and is removed from drupal:12.0.0. Instead, you should use ::renderInIsolation(). See https://www.drupal.org/node/3407994
Triggered by:
* Drupal\Tests\geo_entity_address\Functional\AddressFormsTest::testCrud
/var/www/html/web/modules/contrib/geo_entity/modules/geo_entity_address/tests/Functional/AddressFormsTest.php:86
finn lewis → credited ekes → .
finn lewis → credited ekes → .
amateescu → credited ekes → .
Thanks for filing this and for the fix.
Is there a scenario where old URLs should be supported too? In other words, should be change the regex to the new format or should we just accept the new format as well?
I think I was planning that (?:\/episodes) but then didn't follow through ?. As seen in the in the update from Buzzsprout themselves it's only the new format for their content. In our case however, there will be entities with the old pattern that have it in the field with the old URL, no point in updating that.
OK a bit more than the minimum change to the regex. Add the test and user feedback too.
This patch is specific to configuring for Buzzsprout. Which is actually really handy for me, as that's what I was doing :-) But it should probably be an example addition to the readme, rather than a replacement for the generic description that there is. I'd guess more specific examples might be good, I've not tried if there are other specific differences.
> Out of memory issue reported in #32 still occurs with Leaflet 10.2.20 using a Search API view. Attached patch fixes it.
So far as I can see this stops loading the field data from the Index Item Item::fieldsExtracted is basically only set once Item::getFields has retrieved them from the backend. So unless something else is calling this before leaflet does, the data will be falling back to load it from the entity. Which is what some use cases are trying to avoid. This will usually work, and you won't notice unless putting break points in. It does not work for some other cases, where the field is on a referenced entity.
If I'm correct that @idebr case means, either something else has added the extracted data (not search_api or search_api_solr code that I see), or loading it (probably also retrieving it from the entity) within Item::getFields takes more memory than directly doing a getValue on the field is probably a Search API issue, not one for Leaflet to solve.
It was an unrelated issue, not caused by this one. Behaviour here is correct.
Noticed some possible issues with workflows publishing. Going to put this on needs work until clarified.
I just went searching for this feature as I yet again forgot that the default theme is used for 'render item' which means I can spend as long as I like configuring the view mode, but I still need to also add a template for that view mode or the default one (which will have some heavy customisation) will kick in and override everything I did in the UI config.
I guess I'm going to quickly make templates again. But for next time:
I'm thinking this option would be useful specifically on the RenderedItem processor. So the option to choose theme would make sense with the choice of which view mode to use. Just once? Not for every view mode?
Hopefully clarified.
Serivce name typo should also be fixed. Tests pass I think.
Update 📌 Allow CRUD operations for Redirect entities in a workspace Needs review works.
Adding a patch version of the MR for a patch ready snapshot.
> When content gets published to the default workspace, the redirect_path_update hook works as before and handles the generating of the redirect.
This was making sense to me, why would I need redirects in stage environments. That was until I realized we were using https://www.drupal.org/project/pathauto/issues/3283769#comment-15541961 🐛 Allow path generation inside of a workspace Needs work which stops the re-generation of a pathauto alias when the workspace is published (which makes sense, especially because in experiments with paths that include `[node:entity_reference_field:entity:url:relative]/[node:title]` the order of publishing could cause the alias of the parent not to be available in the live space before the reference entity alias was regenerated hence causing the incorrect alias to be generated).
So related issues (as I'm looking) are: 📌 Allow CRUD operations for Redirect entities in a workspace Needs review and #3187274: Redirect deletion in workspaces →
In the MR !9062 now. Which is passing tests.
The MR https://git.drupalcode.org/project/drupal/-/merge_requests/9062#note_352712 against 11.x on #3179199 also fixes this.
In creating the branch https://git.drupalcode.org/issue/drupal-3179199/-/compare/11.x...3179199... against 11.x I ended up with the same code as @acrazyanimal in #9 it truly does fix both #3179199 and #3132022 but rewrote the tests against the present state of the class and found that to make them clear they should be separated I think. The tests are in the first commit, so can easily be run to confirm they do fail with the present code.
@sam152
> Has the scope of this issue grown to include #3132022: Content moderation check does not take into account entity IDs being used by two content entities? We should either try to limit the scope of this issue and tackle the problems separately, or update the issue summary to describe all the scenarios that are being tested and the bugs that are being fixed.
@acrazyanimal
> Thanks @Sam152. Yes I it does look like it covers the scenarios from the issue you linked above. Reviewing the comments in that issue's thread it looks like Alexj12 also increased the scope of that issue to cover the original issue here lol. I'm not sure what to do really. My inclination is to redefine this issue as an all encompassing issue since the tests are overlapping. I will think it over and possibly reduce this and contribute to the other issue.
Looking I'm starting to get convinced https://www.drupal.org/project/drupal/issues/3132022#comment-15708738 🐛 Content moderation check does not take into account entity IDs being used by two content entities Needs work that the query there is just going to throw too many false positives, for this, and the referenced issue, and for the other example I mention. So it might needs tweaking, not just filtering out revision ids.
Looking at this for the first time, having encountered it on a site in development, and it seems that this can not only happen if there as explained in the OP:
A non-workflow non-revisionable entity with a revision id that is the same as another different entity type revisionable entity (previous not default) revision id that is in a non-published state (the path_alias and node example)
but also if there is:
Any (including workflow revisionable) entity with a current default state revision id (published) that is the same as another different entity type revisionable entity (previous one not the current default) revision id that is in a non-published state.
eg: A taxonomy term ID 1, Revision 1 Published; and Node 1, Revision 2 Published (but also a Revision 1 not published).
The $tracked_revisions https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/conte... returned are going to be $tracked_revisions = [ 'taxonomy_term' => [1], 'node' => [2] ] which becomes $tracked_revision_ids = [1, 2] when queried in the table https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/conte... will return 1 because it is catching the tracked, but no longer relevant node revision.
Looking at the base test class ContentModerationStateTest and WorkspacesContentModerationStateTest extend it, only one entity type is tested at a time for this. So it feels like it might be clearer to make a new test that reflects workspaces behaviour more, where several entities of different types are being published at the same time - obviously with some clashing revisions id's included.
ekes → created an issue. See original summary → .
valthebald → credited ekes → .
Think that covered them. The additional Unit test for the Negotiator is left, but might just duplicate what is in the Access Policy test.
Switched to use GroupSitesNegotiator naming, added setting the CacheMetadata. Made this optional. These tested in the Kernel test.
Then changed the AccessPolicy to use the new Negotiator. Updated the Unit tests to reflect the changed internals, and added equivalent Unit tests for the GroupSitesNegotiator.