🇦🇺Australia @acbramley

This is done, the last thing I want to do before cutting an alpha is 📌 Investigate better HTML diffing libraries to replace HTMLPurifier Active

Making the module entity agnostic will come once Node uses the generic revision UI :) (in a 3.x branch perhaps)

acbramley → created an issue.

GH79329918HG

I will test this with our previous use case

acbramley → changed the visibility of the branch 3429862-d11-compatibility-fixes to hidden.

Agree with #13 here - there's also no reason to disable the eslint job you can simply ignore the warnings (it does not fail the pipeline by default) - the same goes for phpcs and phpstan. IMO do the bare minimum to get this in and branch out into other issues. Unblocking D11 support would be fantastic.

acbramley → changed the visibility of the branch 2.x to hidden.

Thanks for all the work here, we've got green tests for next major and minor and only a small number of phpstan failures on next major which we're ignoring for now (these can be fixed in a follow up but is not a priority).

Here's what I settled on:

1. Fail phpcs/phpstan on current version
2. Allow failures on next major/minor for phpstan
3. Run tests on next major/minor
4. Turn off SYMFONY_DEPRECATIONS_HELPER on next major

acbramley → changed the visibility of the branch project-update-bot-only to hidden.

@ericgsmith source and target definitely make more sense to me. I'd be happy with that.

acbramley → created an issue.

Thanks

@ankitv18 those changes were for Drupal 8, we don't support Drupal 8 anymore.

I'm find this hard to follow as questions are being asked and then more changes are being pushed.

I do not feel like we need to run tests on previous minor/major. This is just busywork maintenance to keep the tests working. For a module that's already undermaintained for how widely it is used, I would prefer to not have this overhead.

CORE_PREVIOUS_PHP_MIN: 8.1

I've bumped the minimum PHP already on 8.x-1.x to 8.1, is this still needed?

Basically, I agree with #31

I'm not a huge fan of these column headings, and as stated already in the comments the A/B labelling may not make sense in other languages. With that being said, I don't really have any good alternatives but don't want to commit something before we get more of a consensus.

acbramley → created an issue.

There are no PHPCS failures on HEAD https://git.drupalcode.org/project/diff/-/pipelines/176683

These changes are causing many failures in the MR.

The only things failing in gitlab CI are eslint and stylelint now.

The baseline changes are to fix failures that also happened on the current branch, not just next

Yes but then it started failing phpstan. HEAD was updated and fixed to include this with the PHP 8.1 requirements.

naughty bot

acbramley → created an issue.

Please update to use an MR.

Please update to use an MR and add tests.

PHP7 is EOL. This will not be merged. We can however specify PHP 8 requirements in composer.json - this will be done on the 2.x version of the module.

PHPStan is failing.

Duplicate of ✨ Only show image thumbnail for image plugin when changed Needs work

This is being actioned in other issues.

Please update to use an MR.

Please update to use an MR.

This feature will not be accepted into the module. Feel free to roll your own module or use a custom approach.

Please update to use an MR

Please update to use an MR

Please update to use an MR

I have no context of what this issue is trying to achieve. Please fill out the issue summary.

Please roll this into an MR.

What is this change achieving? How is this a bug?

Needs tests and an MR. Patches are no longer accepted in this project.

This will be resolved in 📌 Use type-hinting on deprecation warnings for loadRevision() Closed: duplicate

As discussed in slack, I'd like to keep enforcing phpstan passing. As a maintainer, I'd rather the MR pipeline fail outright than having to check every time if any regressions have been introduced there. The problem is now of course that the next major runs are failing because, well, code is deprecated there that isn't in our current major. It makes no sense to me to fail phpstan on next major.

The current MR also introduces new failures on the current major with the baseline changes so back to NW.

@deepak5423 did you not see the existing MR? Please close yours. Back to NW to address #15

acbramley → changed the visibility of the branch 3127124-t-fn-replace to hidden.

bbrala → credited acbramley → .

Please use a merge request, patch files are not accepted on this project anymore.

So the failure is because the allowed_values settings have completely changed. They are no longer added via a textarea but a tuple of key/value pairs with individual text fields.

I can't see any other tests that use $this->fieldUIAddNewField for a field like this. I'm not sure it's even possible.

In that case, we should probably just do a FieldStorageConfig::create and friends to bypass the new UI.

Tests are failing

🐛 t() calls should be avoided in classes RTBC contains the t() fixes.

Would be good to get this merged so gitlab CI can go green :)

@andypost thats only in gitlab CI, which is also failing on HEAD. I was trying to get some quick fixes through but obviously it needs more work so I will split that stuff into a separate issue.

larowlan → credited acbramley → .

@larowlan sweet! Yeah definitely need a test, just wanted to get agreement on the approach. I was also trying to think of a better name for the config key than just "operations"

Example pushed to the MR

I think we should hard code 'update and delete' for now to stop the bleeding and then revisit for configurablity
I was just thinking for BC purposes we should keep all operations by default.

acbramley → created an issue.

Was that an accident @andypost?

Merged into the new 4.x branch which only supports Drupal 10. Since 8.x-3.x uses the old branch name format and was the D8 branch I wanted to leave that behind.

A 4.x dev release and alpha are to follow.

If the schema has already been broken, do we need to migrate any data? It never would have worked to begin with?

MR up with a straight uninstall + reinstall update hook.

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

Does this bug still apply to the latest version of Drupal core?

To me, this seems like quite an edge case - why allow creating and editing media without being able to view it?

But since the content of the given field is always unique(in my case), it has no sense in confusing people with giving them access to all the other media files.

Maybe media library isn't appropriate then?

Postponing looking for a bit more information as to whether this should really be fixed in core.

Based on the lack of feedback since #16 and given this is not unique to this class I can't see how this should be classified as a bug.

Swapping to a task, IMO we could/close this if no one can answer #16 within a couple of months?

bbrala → credited acbramley → .

acbramley → created an issue.

Thanks! Committed to 8.x-1.x.

Don't remove the tag if tests haven't been added please.

Amazing, thanks everyone! And thanks a lot @mark_fullmer for including this in a release already :)

Updated to default to TRUE.

6 years later and I still strongly agree with #23

It makes it very easy when traversing class heirarchy trees in phpstorm for example. There may be icons but the word makes it much easier. This is common practice in almost every PHP library I use.

@mahtab_alam did you mean to change the status? This is still a work in progress...

Tests are now passing but they should really be expanded to cover more functionality.

Picking up from #24 and getting this into shape in a new branch on the fork.

Will setup Gitlab CI and port tests to BTB

acbramley → changed the visibility of the branch 3145500-drupal-9-readiness to hidden.

This should be a support request, it definitely works if things are set up correctly :)

I have used both plain text and HTML based emails across many projects, Drupal versions, and email clients all the way back to D6 and I've never seen this issue.

Considering the disparate reports and lack of detail (e.g steps to reproduce) I'm inclined to close this as won't fix or at least change it to a support request?

If people are still seeing this issue, please at least include the email client you're using. I see 1 year ago in #62 it was reported in alpine plain text (never heard of that) and 4 years ago in Outlook. Does it still happen in Outlook? Is it only certain versions? Certain settings? Etc.

This feels like it could fall into the same category as a bug report that styling is broken on some obscure/ancient browser...

Not sure is this is truly jsonapi or more caching

True it could exist in other areas, I'm not sure if other parts of the caching system cache things like this though. The issue really stems from that fact that we get a cached item (the normalized fields) which already contain max ages, add stuff on to that same cache entry, and then re cache it with a mix of new and existing data.

Big +1 to just having it as it is now (i.e #[Hook('hook_form_myform_alter')], not #[Hook('hook_form_FORM_ID_alter', form: 'myform')] - I don't even know how that'd work...

Added some feedback.

📌 Add #cache['downstream-ttl'] to force expiration after a certain time and fix #cache['max-age'] logic by adding #cache['age'] Active reports this exact issue in #11

I've confirmed that the drifted max age bubbles up to DPC resulting in the entire page being cached for too long as well.

I just created 🐛 ResourceObjectNormalizer::getNormalization can result in max-age drift when different sets of fields are requested Active which is basically the same as #11 - the drift can still affect things even without DPC though.

acbramley → created an issue.

Rerolled and squashed the MR against 11.x - now green again.

We still need to decide whether we keep the upgrade path or not. IMO the code is there and working so it seems like it'd be good to keep but if people feel otherwise then I'm not too fussed tbh...

Amazing! Thanks guys

@Luke.Leber great points, fully agree with the generic entity stuff, I just wish Node was using that already so it was even easier.

I am keen to explore other HTML diffing libraries, when I last checked the one this module uses is quite outdated?

@sokru you need to have at least 1 other secondary tab to trigger it.

Re #14 and #15 this has to be generic to all content entity types, not just node types. Currently the MR sorts by weight, then label. IMO the after patch is closer to what is expected than the before so putting this back into needs review.

A.k.a. using the NodeType sort() function?

What is that?

Please stop uploading patches to this issue. It is causing problems with the NR bot and making this even more tedious to try and get over the line.

I completely agree with #164 - let's get this in and polish it further in follow ups if needed before we get to another 500 comment issue.

It's great to see more movement on this problem space but this new issue has raised a few questions for me. Quick background: I have been using Hux since it's inception, and am arguably one of its biggest users and supporters seeing as my colleague wrote it :)

We now have several implementations of Hux like OOP hooks, it would be great to get a consensus on what one we're going forward with and close out the other MRs/issues ( https://www.drupal.org/project/drupal/issues/2237831 🌱 Allow module services to specify hooks Needs work for example)

I'm also curious what the differences (if any) there are with this solution when compared to Hux. It seems quite similar but I haven't had a chance to review the code yet.

Thanks for all the great work so far!

This is great! Anything to improve these tests is much appreciated, maybe we should turn 📌 Tests extending jsonapi's ResourceTestBase are extremely hard to debug/maintain/deal with Active into a meta?

Does this not need to go into 11.x first? mr is against 10.3.x