Fixed.
Thanks, merged. Release forthcoming.
Great, looks good. A couple of things:
1. Do we still need the existence check in save? As we've disabled path_entity_translation_create() the pid is no longer unset, so parent::save() should just save the existing entity.
2. The comment on disabling the core hook needs to explain why we do it. Something like
The core path_entity_translation_create() hook attempts to save an alias per translation by unsetting the pid of an existing alias. As we reset the language of the alias, this will just create duplicated (or conflicting) aliases, so we disable this behaviour.
3. Could you squash the history down to one commit?
> Yes will update the MR to respect OOP.
Sounds good.
> But the second part:
Some observations and assumptions, do correct me if you think I'm wrong:
> 1) The URL Alias field must set as untranslated when using this module.
What do you mean specifically? I notice that path_entity_base_field_info() sets the pathbase field to translatable, but I don't know if that really affect us as we're dealing with alias.
path_entity_translation_create() seems to be the cause of our woes, as it unsets the pid of the alias (but keep the alias) when creating a translation. Maybe disabling that will make things behave.
I don't like just form_altering the field away with a general form_alter. I'd rather get closer to the source by overriding the field widget, if we must do it.
@drunken monkey
> no-one was engaging in the debate anymore
Well, apparently it wasn't a discussion, but a vote. I'm still waiting for someone to explain to me why Search API is so special that it needs to deviate from what is now The Drupal Way, apart from "well, we've already done events" and "everybody else is doing it".
I can't imagine that anyone thinks that having two different ways to accomplish the same thing spread over the contrib ecosystem is a particular good thing.
But if nobody wants to discuss it. *shrugh*
@mkalkbrenner
And you wouldn't be able to bundle it all together in one hooks class, if the related modules did it the Drupal way?
There's obviously a challenge here, but I don't think the proposed solution is the right path.
For starters, \Drupal shouldn't be used in OO code, the relevant services ought to be dependency injected. But directly querying the database to check for duplicate couples the storage class tightly to the database schema. $this->loadByProperties() ought to work for checking for duplicates.
Secondly, hiding the path on translations hides the fact that the translation *does* have an alias. I'd rather skip that part and live with the fact that editing the path on the translation also edits the original language alias.
@drunken monkey
> I don’t think that OOP hooks using events internally is really an argument, since that’s immaterial to anyone using or defining them.
It was more of a technical argument. But if that's the case (I'll admit to not have bothered looking into it yet), one ought to be able to use an event subscriber to "listen" for hooks. Not that I'd recommend it.
> general PHP best practices
Speaking of practices, one of the most important is "be consistent". If there are two ways to solve a problem, always pick the one that's already in use in the project. The Drupal project has chosen OO hooks over events, some might argue that they ignored said rule, but I don't think it was done without thought. And when core choses a way, contrib ought to follow in order to safeguard the consistency of the project as a whole, unless they can provide a convincing argument as to why this would be a bad thing.
This isn't about whether one is better than the other, it's a matter of doing things the Drupal way when you're in Drupal land. I much prefer Laravels interface naming, but when I make an interface in a Drupal module, I use Drupal naming convention.
Well, let me submit the opposing viewpoint then: I think search_api should use OO hooks. Reasoning, in no particular order:
* Drupal Core has decided this is how we do things in Drupal land.
* search_api is a prominent project, and its codebase is read by many an aspiring developer, I think it ought to show the proper path.
* The ergonomics of OO hooks is, IMO, better.
* OO hooks is AFAIK based on events anyway.
@jsacksick
Release 3.2.0 pushed.
@anybody
Not very actionable input, but it's my impression from the time since my last comment two years ago that the language system still have some issues. Like the "why did it update all the language configuration files" thing that crops up now and again.
But this is hard to nail down, as developers are likely to just utter a curse, perhaps revert some files, and push out the feature that the customer requested, rather than spending hours trying to debug why it happens.
I think the problem is that those most interested in getting this merged lack the required insight to write proper tests.
Where does one see that "missing test coverage"?
This issue is two years old and people are starting to maintain MRs to patch from. Isn't it about time it gets merged?
It is. I haven't worked on a site that didn't use pathauto in something like a decade, if not more.
language_neutral_aliases works on a level below pathauto, fixing the aliases that pathauto creates.
In general, one can assume that modules are compatible, unless stated otherwise. Trying to maintain a list of compatible modules would be a major effort.
There, added tests and fix.
Xen → changed the visibility of the branch 3440302-replaceoriginalhook-not-discovered to active.
Xen → changed the visibility of the branch 3440302-replaceoriginalhook-not-discovered to hidden.
Ah, on second thought, adding Alter and ReplaceOriginalHook to hux_auto_test is more proper.
> So what youre saying is it theoretically didnt work until you added a Hook/Alter to any method in the same class, not just the replace method.
Exactly. Or rather, not theoretically. I added the #[Hook('node_insert')] annotation to the same disable method that had the ReplaceOriginalHooks. That triggered the autodiscovery. So the method is both a replacement and a hook implementation. I didn't test whether that makes hux call it multiple times, but as the method does nothing it doesn't really matter i my case.
> In terms of fix, i believe we just need to update \Drupal\hux\HuxCompilerPass::getHuxClasses. We dont need to update the existing Replace test fixture.
Yeah, it was for demonstration purposes only. I do hope Gitlab will let me force push...
> Lets create a new autodiscovery-only Hooks class fixture, where it only has ReplacementOriginalHook implementations
For completeness sake it ought to have 3 classes, one for each autodiscovered annotation.
I'll whip something up...
> The example code doesnt look right. (class namespacing, attribute' moduleName param)
I replaced the module names as they're irrelevant for the issue and specific to the project where the issue was discovered, just as I removed the usual commenting you'd find in such code.
> There are also tests for this.
ReplacementOriginalHook isn't tested with autodiscovery of hook classes. The hook class is registered manually as a service: https://git.drupalcode.org/project/hux/-/blob/1.5.x/tests/modules/hux_re...
Note how the test is failing in the MR where I've changed it to autodiscorvery.
It's still relevant.
Drupal\Core\Database\StatementWrapperIterator and Drupal\Core\Database\StatementPrefetchIterator (and the respective classes they're deprecating) both implements Drupal\Core\Database\StatementInterface.
For the former, if the second argument to fetchAllAssoc is a string, it'll assume it's the name of a class and use FETCH_CLASS:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
The latter delegates fetchAllAssoc to fetch:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
Which will throw an exception if the fetch style is a string:
https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...
StatementInterface describes the $fetch argument as:
* @param $fetch * The fetch mode to use. If set to \PDO::FETCH_ASSOC, \PDO::FETCH_NUM, or * \PDO::FETCH_BOTH the returned value with be an array of arrays. For any * other value it will be an array of objects. By default, the fetch mode * set for the query will be used.
Which doesn't exactly describe StatementWrapperIterator (it doesn't say that supplying a string is interpreted as a class, even if StatementWrapperIterator is returning an "array of objects"), but StatementPrefetchIterator is clearly not upholding the contract, as anything but an int corresponding to a fetch mode constant throws an exception.
The interface needs to be more clear on the expected behaviour, and StatementWrapperIterator and StatementPrefetchIterator should interpret the arguments in the same way.
Updated info file.
Committed the latest patch.
Committed, thanks, release coming up.
Committed, thanks.
Sorry for the late response, I've had some issues with notifications.
I've updated to the latest version and I'll see how it looks.
Ah, excellent idea. I've updated the MR, but haven't had a chance to test it.
@mfb
Nice, but there's a related problem: the feature that collects log messages produced in the request and add them to the issue still get the old trace. The PHP options that you mention should be able to take care of this, but it means that this fix is half baked. Should it (and can we) clean the log messages too? If the PHP options is needed to clean the log messages this fix is sorta redundant. I think raven should either fix both, or instruct users to set the options instead.
Taking the liberty of upgrading this to major, as this can potentially leak sensitive information to Sentry, as Exception::getTraceAsString() includes string parameters in the backtrace.
I've re-rolled against the latest version and taken further liberty in removing the configuration option. As far as I can tell, Sentry/Raven always produces their own backtrace, which can be configured to include parameter values or not, so the backtrace in the message is redundant and fills up the issue page at Sentry pushing the nicer looking native backtrace down.
No interdiff as 3326662-3 doesn't apply to HEAD.
Added in 3.2.0 →
I've added an option to use it on non-user profile forms.
Might change it to a permission, but that would require a major version bump.
Created a fork with a suggested solution.
I don't know why something thinks I shouldn't get issue notifications anymore.
Anyway, I've committed, and added a new 3.1.0 release.
Boolean options under options is not interpreted as strings, so make a note of this and fix the example values.
This patch fixes the accessCheck issue that rector can't automatically fix.
Together with the patch from #3, this makes the module D10 compatible.
Tried running the tests on PHP 8.2 and Drupal 10 locally, and they ran fine. Think the failures might be a fluke.
> Why on earth were these updated?
Turns out that, when configuring a role to have access to a text format, it adds a config dependency from the role to the text format. So that explains why the role configuration was touched. But I'll still consider it a bug that it changes the untouched label from English to Danish.
Here's another odd consequence of the current system: Have a site with `da` as default language, enabled a module which set `da` as langcode for all config, even though the current texts are English. OK, they're translated by locale, so things work.
Now I create a text format and export it. Which causes something to update `user.role.authenticated.yml` and `user.role.anonymous.yml` to contain Danish strings. Which makes the language code of the file more correct, but as far as I understand (someone please correct me if I'm wrong) this means that the user roles names will *not* be updated if the translation changes? I think this goes against the intention in the graph of #81. Why on earth were these updated?
Anyway, while I understand wish to make the "core" configuration english and layer translations on top, it'll have another problem: configuration without an english default. If a danish user on a danish site (with danish as the default language) creates a new role, it's obviously in danish. As it's a new role, we don't have an english string to put in the configuration.
Is a dual mode feasible? When configuration is imported on install, it's installed with `en`, but if a user saves a configuration page, it'll be assumed to be the default language, unless there's already a translation overlay?