The MR goes on top of the one from 🐛 Guzzle exceptions in Whitespace provider are not caught Active .
Entity Share Bypass Fields sounds useful, but it looks like what it does is skip field values entirely.
What this issue is about is importing the field value, but not following the reference to the target content.
For example, I am pulling my node about Cake, which has a CTA to the 'Recipes' node. I want to import the reference *value* of the CTA field, but I don't want Entity Share to *follow* the reference and pull in the Recipes node.
@divyat There's no need to take screenshots of code being applied -- we can see the diff on gitlab and gitlab tells us that the MR can be merged cleanly.
Added checking of the error code.
I'm having it return an empty array at the moment. I'm not sure if this is the best thing, as that means the controller will show:
No collection schedule found
We could not retrieve the collection schedule for this property. Please check this is a domestic property, and contact us if the problem continues.
-- is that the most accurate thing to show the user?
It was a domain problem -- I was connecting to an environment that didn't have up-to-date collections.
Catching of Whitespace exceptions has been added to 🐛 Guzzle exceptions in Whitespace provider are not caught Active , but the code here still needs to check for the case where the returned XML has:
<Collections i:nil="true"/>
I can't figure out how to get that by inspecting the $collectionsXml from:
$collectionsXml = $xml
->children('s', TRUE)
->Body
->children('http://webservices.whitespacews.com/')
->GetCollectionByUprnAndDateResponse
->GetCollectionByUprnAndDateResult
->Collections;
SO says I should be able to cast $collectionsXml to bool and get FALSE, but that's not working.
Just noticed the MR has a @see to this issue.
I'm removing this -- @see to an issue is not necessary, there is git blame for that. @see to a URL should be used where there's a need to link to something like an external technical document.
> it f... up every design due different user agent style sheets. This is something that everybody has to fix in every project.
Things have maybe changed since that comment 4 years ago.
I've tried this fix on both core's Olivero theme and my project's theme, and I can't see any difference visually.
Using type=search is important for accessibility, as assistive technologies will know to present the form element differently. See https://www.magentaa11y.com/checklist-web/search/ for instance.
Added a helper for making SOAP calls, which handles SOAP errors and re-throws them.
I should move the catching to a helper method, as this should happen further on too.
> I’d just need some high level steer as to what I’d need to do
That unfortunately is what is missing on this issue. We can't agree on what the correct behaviour is.
> Do you know which version of the Whitespace API is in use for your site?
No, I'm afraid.
> Do you have the API documentation?
No -- I've looked on the Whitespace website for documentation on their API, and I can't find anything (and it crashes!)
joachim → created an issue.
Pushing an attempt at a fix, but outputting an error message when we catch an exception contradicts the error message in twig:
<p class="alert alert-info">No properties found for postcode {{ noresults }}. Please check this is a valid postcode and search again.</p>
as both will show in the case of a Guzzle exception.
Not sure how best to fix this.
Perfect, thanks!
(Remember to set issues to 'Needs review' when they're ready! :)
JS test failures are unrelated. RTBC.
All of the stuff that says 'this is changed from D7' needs to be removed.
joachim → created an issue.
joachim → created an issue.
This isn't changing the hardcoded instance that should be removed by 📌 needless code in showForm() Active .
Thanks for reporting back!
+++ b/core/modules/views/src/Plugin/views/join/JoinPluginInterface.php
@@ -14,13 +17,13 @@
- * @param $select_query
- * An select query object.
+ * @param \Drupal\Core\Database\Query\SelectInterface $select_query
+ * A select query object.
* @param $table
IMO fixing this typo is out of scope here.
Besides, the docs need more detail -- what about the select query object? What is it here for, what does this method do with it?
> Sometimes I wonder why Plugin API is not leveraging Symfony's dependency injection component by using service collectors for plugins and reinvents the wheel at some point.
My feeling about that is that the service container becomes a bigger and bigger dumping ground.
And services need to obtained by anything, potentially. Whereas plugins have a dedicated manager you ask for the plugin. I could envisage each plugin manager having a dumped PHP helper which handles instantiation and DI.
> There are already ways of decorating a plugin if you really want to
What methods are out there in the wild?
> An important thing I miss since we replace Doctrine Annotations by PHP attributes is, that one is not able to add an arbitrary definition key anymore
See ✨ Allow attribute-based plugins to discover supplemental attributes from other modules Active .
Revisiting this a little.
I think one of the problems with decorating plugins is that plugin classes tend to have a LOT of methods. Services that get decorated are fairly slim. When we have a stack of services, they're often classes with only 1 or 2 methods. We have some with more methods -- e.g. WorkspacesEntityRepository, which has to have 5 methods which just relay the call to $this->inner.
But plugin classes often have FAR more functions.
BlockBase has over 20.
Views' PluginBase has 32, and that's only generic base -- FilterPluginBase has 57!
That's a TON of boilerplate to add to a decorator class.
There's the technique of adding a magic __call(), which some services do, e.g. AnnotationBridgeDecorator.
But magic call is slow -- according to this, 5x slower on php 8: https://gist.github.com/bwaidelich/7334680
So we'd probably want each plugin type to provide a trait which is all of the base class's methods, calling $this->inner, for decorator classes to use so they can just implement the methods they care about.
Or we look at #2754071: Introduce plugin handlers → , and make plugins smaller, more focussed classes, which would therefore be easier to decorate.
I'm +1 on this.
But:
> *single-use* providers
What happens when a single-use provider is converted to a multi-use provider because we add another test which uses it too?
> However, if we mandate that every set of arguments is keyed by a meaningful string then this is super useful.
That is super useful, but it's code, not documentation. Out of scope for this issue?
> If we follow the core standards of documenting @return and @param tests will have a lot of boilerplate that does not add much.
Documenting both the @return of the provider and the @params of the tests is writing the same thing twice, and redundant, but *one* of them should be documented!
This seems to duplicate part of #2057905: [policy, no patch] Discuss the standards for phpunit based tests → . Is it meant to be getting split off from that?
-1 from me anyway for the @group tags as described -- they just duplicate the folder structure, so they don't add anything.
PHPUnit annotations are now attributes -- IS needs an update for this.
> Use @group Drupal to allow filtered testing.
> Use @group Namespace or similar for further filtering. e.g: @group Entity
What is the point of this? If all the Drupal core tests are in core/tests, then you pass that as a parameter to PHPUnit instead of a @group.
If all the tests for core/lib/Core/Foo are in core/tests/Foo, then you pass that.
Given how much people are complaining about writing docblocks for test methods in 📌 [policy] Remove the requirement for doxygen for test methods Needs work , why are we adding superfluous @group tags?
You'd need to add that to the plugin's options and functionality.
Thanks for reporting back!
In that case, I'll close this.
That contrib project https://www.drupal.org/project/plugindecorator → now says it's obsolete, but there's no explanation of why or whether something else has replaced it.
> There's no system in Drupal or Entity API to determine which of an entity's properties appear in the form, or how to present them.
That was 13 years ago though!
There IS that in D8+! :)
I'd forgotten about that issue -- thanks for the reminder.
I'm not sure an enum for a boolean value is useful. The main advantage of an enum over constants is that you get a type that you can enforce. But with TRUE/FALSE, we already have the boolean type.
For entity paths, the problem happens deep in core's UrlGenerator.
Tome's EntityPathSubscriber calls it in the
$event->setPath(parse_url($url->toString(), PHP_URL_PATH));
This has the base URL for the actual site:
$base_url = $this->context->getBaseUrl();
and so returns "/drupal-contrib/web" (I've switched to a different site to experiment since my last comment!)
UrlGenerator then does this, prepending the base URL:
if (!$absolute || !$host = $this->context->getHost()) {
$url = $base_url . $path . $query . $fragment;
Then tome's EntityPathSubscriber get this URL for the entity:
> '/drupal-contrib/web/node/1'
Ah, I hadn't spotted that!
The exception message will be wrong in that scenario though:
> 'The "%s" form handler of the "%s" entity type specifies a class "%s" that does not extend "%s".'
because it's not been caused by a form handler.
I've tried adding the SiteRootOutboundPathProcessor I posted in an earlier comment, and that doesn't actually fix it -- I don't seem to have pages for path aliases exported at all with that in place!
This also isn't working when I try to generate to a domain.
With my site at http://localhost:8888/joachim-blog/htdocs on my local, and generating to https://joachim-n.github.io/, I get:
- the root of the generated at http://localhost:8888/joachim-blog/html/joachim-blog/htdocs/index.html
- the CSS links in the generated HTML have URLs like this: href="/joachim-blog/htdocs/themes/custom/joachim_blog_theme/fonts/metropolis/Metropolis-Regular.woff2"
Right, so basic_auth works with jsonapi routes because jsonapi module does this:
$routes->addOptions(['_auth' => $this->providerIds]);
and so basic auth won't work for private files OOTB.
But we could make it do that -- we could add code to the server module which enables basic_auth for files if the module is enabled.
There is a version of that on 11 though: https://api.drupal.org/api/drupal/core%21modules%21system%21system.token...
The check in core/lib/Drupal/Core/Entity/EntityTypeManager.php is not what I was expecting, and it's a good idea because it'll give a better error for people who use the wrong sort of form class as an entity handler.
But it won't catch the case where you use _entity_form directly in a route definition.
Pushing what I added to my local as a start for someone else to pick up!
joachim → created an issue.
Ahhhh checkboxes. ARGH. Maybe I tested on a form without unselected things!
In that case, the filtering should be happening much earlier -- on the two parameters to the intersect for one thing, but probably even in the form, as the method you're changing is supposed to be working with config (where AFAICT there won't be stupid empty values), and is only being called from the form as a hack.
joachim → created an issue.
joachim → created an issue.
This issue is still relevant.
This seems to sort of work now -- provided you don't add fields later: 🐛 'Searched fields' option to search all fields if none selected should not save them Active .
joachim → created an issue.
I think the way I'd approach this -- with a very incomplete understanding of how this module works!!! ;) -- is to introduce a plugin which provides actors.
So we'd have:
- entity actor provider
-- with deriver for all content(*) entity types
- sitewide provider, where the actor's details come from config settings
(*) I could see actors being config entities too. For instance, we might want an actor which is 'Latest Articles from MySite.com', where any new article node automatically makes a fediverse post, and so the actor is the node type.
I also got this when no fields were selected because of 🐛 no options show for 'Searched fields' if the only field is 'rendered_item' Active .
> As for my merge request: It does indeed allow you to use computed field without ever attaching it to the given content type. I don't consider that to be a deal-breaker, though, as it allows adding to the view without custom code, and computed fields themselves work the same way
Yeah but that isn't at all how Views works. Views data declares fields (and which plugin they use), not plugins where you have to select the field in the UI.
Why do we need to array_filter(array_intersect())?
I thought I had test coverage for this one :(
I never saw this when I was creating finders in the UI.
Though it's possible my sandbox has the discovery cache set to null...
Anyway, clearing the whole discovery cache seems rather heavy-handed.
See my comment #8.
I don't see what I can add to the IS to make it clearer.
Drat, I searched for duplicates too!
That issue has no work, and this does, so I think it makes more sense to keep this one.
From that issue:
> Another issue here is that EntityPublishedTrait::isPublished() does not respect those constants. That could be fixed like follows.
Maybe best for a follow-up?
This is the case now.
That seems like overkill to me. It's also less clear than saying in words what is needed.
Developers using this class will be creating custom entity types, so they should know what an entity keys property is.
Could you see if 🐛 Editing a path alias on the server to point elsewhere causes a broken state with aliases on the client on pull Active fixes your problem?
-
+++ b/modules/entity_share_client/src/Form/PullForm.php @@ -605,6 +605,32 @@ class PullForm extends FormBase { + // Invoke hook_entity_share_client_channel_query_alter() to give installedIf we invent a hook, it needs to be documented in an API file.
-
+++ b/modules/entity_share_client/src/Form/PullForm.php @@ -673,6 +684,12 @@ class PullForm extends FormBase { + if (str_starts_with($tracked_filter, 'custom_filter_')) {Seems like there magic going on here that needs to be documented.
But given we pass in 'tracked_custom_filters' empty, why would a module implementing the hook add something WITHOUT this prefix?
I don't know what I can respond to #6. I think it could be clearer. I don't see what's the downside of clarifying the error text.
Left some comments.
benjifisher → credited joachim → .
There's a lot of unrelated churn in the changes. Was this done with AI?
I think that's because that field type plugin doesn't implement mainPropertyName().
Without that, JSONAPI defaults to 'value'.
How would a tagged service work?
> Which is where things start to get interesting. This currently only has access to its own definition. but that callback is called on the whole definition and is capable of implementing back-references (see file_field_views_data_views_data_alter() for example and core_field_views_data() for term entity reference fields.
This precise point has come up at ✨ provide Views reverse relationships automatically for entity base fields Needs review -- handler classes for field type views data need to have one method for the field table, and another, optional one, to allow adding to the whole of the Views data.
I've had an idea for a clean way to do this. We would need get 📌 Allow @FieldType to customize views data Needs work in, and then the classes that provide Views data for each field type can implement two methods:
- one to add field data for the field table, with &$field_data as a param
- one to allow the class to add data ANYWHERE, with &$data as a param -- most field types would not need to use this
Thanks!
Unfortunately, I didn't read back far enough -- there is work needed on the bundle label too.
And yes, this needs a rebase on 11.x too.
The IS needs updating too.
We really don't need docs on __construct -- they will just duplicate the class docs.
This constructor sets up the form builder for compatibility, disables
* pagination (to allow full drag-and-drop), and stores the weight key
* if it is defined in the entity type.
This is useful, but put it as an inline comment at the top of the method code.
The docs in this MR all seem very verbose -- it's not AI generated is it?
Isn't 'Add directory facets type' going to take you out of the content area and into the structure area?
The UI texts aren't clear when the host and target entity type are the same, as with taxonomy parent field:
> “Taxonomy term using parent” — “Relate each Taxonomy term with a parent set to the taxonomy term.
Needs a rewrite.
In the meantime, I've converted the most recent patch to an MR, rebased on 11.x.