Problem/Motivation

Before installing this module I had my site set up to show the administration interface always in the same language using the language detection option "Account administration pages". After installing this module, whatever I tried I never got the list of prior revisions to delete as well. Until I enabled Xdebug and noticed that "$langcode" in the function "getPreviousRevisions" was getting the wrong current language.
It took a while to figure out why until I disabled that language detection option.

Now I'm not sure: is this a "node_revision_delete" bug or a core bug because one would expect when using languageManager->getCurrentLanguage() you would actually get the language set in the URL and not the current interface language (which can be manipulated by that language detection method "Account administration pages").

Steps to reproduce

  1. Install site using languages and translations. Install at least one extra language.
  2. Activate the language detection feature "Account administration pages" and make sure it's positioned at the top of the list. Also this language detection only works if you also set the preferred language in the user profile.
  3. Create a node with several revisions
  4. Install this module and try to delete a revision which has older revisions as well.
  5. Notice it will not display the details box with prior revisions to delete.

Proposed resolution

TBD: fix module or move issue to core issue queue

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#11 apply-patch.png18.41 KBhimanshu raj
#11 after-apply-patch.png57.14 KBhimanshu raj
#10 befor-apply-patch.png68.18 KBhimanshu raj
#9 after.png59.75 KBangel_devoeted
#7 3167246-delete-previous-revisions-fix-7.patch1.09 KBsumit saini
#3 3167246-2.patch991 bytesaimevp

Issue fork node_revision_delete-3167246

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

aimevp created an issue. See original summary.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented

Thanks for reporting and for use the issue summary template, I will try to take a look to this in the next days.

aimevp’s picture

aimevp
Primary language Dutch
Location Belgium
commented
Status: Active » Needs review
StatusFileSize
new 3167246-2.patch991 bytes

I was able to solve my problem using following patch. By default getCurrentLanguage() uses "LanguageInterface::TYPE_INTERFACE" which is the interface language. Using "LanguageInterface::TYPE_URL" assures we have the language set in the url (which works for me since I'm using the language detection method URL).

This certainly needs review however because I'm unsure it's to safe to just assume that TYPE_URL is active on every multilingual Drupal setup. Extra logic may be needed to detect the real current language.

adriancid’s picture

adriancid
Primary language Spanish
Location Qc, Canada 🇨🇦
commented
Status: Needs review » Closed (outdated)

Closing because the current development is being made in branch 2.x

sumit saini’s picture

sumit saini commented
Version: 8.x-1.0-rc2 » 2.0.x-dev
Status: Closed (outdated) » Needs work

This issue is still reproducible in 2.x version when negotiation for content language is different from interface language.

sumit saini opened merge request !54

sumit saini’s picture

sumit saini commented
StatusFileSize
new 3167246-delete-previous-revisions-fix-7.patch1.09 KB

patch file of the MR incase someone needs the same fix.

sumit saini’s picture

sumit saini commented
Status: Needs work » Needs review
angel_devoeted’s picture

angel_devoeted commented
StatusFileSize
new after.png59.75 KB

Hi @sumit saini , thanks for the fix!
Tested MR !54 and it works as ecpected. The missing "Delete prior revisions" option shows up again as expected and the code change looks solid to me.
Screenshot attached.

result

himanshu raj’s picture

himanshu raj
Primary language English
commented
StatusFileSize
new befor-apply-patch.png68.18 KB

Hi @sumit saini , thanks for the fix!

I was able to reproduce the issue on a multilingual setup with the "Account administration pages" language detection enabled.

Steps performed:

- Enabled multiple languages and set French as the administration language.
- Enabled "Account administration pages" language detection.
- Created a node with multiple revisions.
- Attempted to delete a revision.

Before applying the patch:

- The revision delete page only displayed the default confirmation screen.
- The section for deleting prior revisions was not shown.

After applying the patch(MR !54):

- The "Delete prior revisions" section is now displayed correctly.
- Previous revisions are properly listed and can be selected.

Conclusion:
The patch resolves the issue and works as expected.

himanshu raj’s picture

himanshu raj
Primary language English
commented
StatusFileSize
new after-apply-patch.png57.14 KB
new apply-patch.png18.41 KB
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

I think we need to allow the langcode to be passed into \Drupal\node_revision_delete\NodeRevisionDeleteInterface::getPreviousRevisions() as this is called from a Drush command too so I'm not sure what the we'd expect to happen there. The drush command needs to allow the user to specify a language code and I think we should also pass the langcode into the batch as well.

Plus we can update the entity to select based on the langcode, revision id and revision translation affected and not have to any entity revision loading which will make this must more performant.

alexpott opened merge request !56

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs work » Needs review

  • alexpott committed d64706f7 on 2.0.x 
    fix: #3167246 "Delete prior revisions" doesn't work if site builder uses...

  • alexpott committed 6e9b0061 on 2.1.x 
    fix: #3167246 "Delete prior revisions" doesn't work if site builder uses...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

alexpott closed merge request !56