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?