IchHabRecht / content_defender

Define allowed or denied content element types in your backend layouts
GNU General Public License v2.0
81 stars 38 forks source link

Cannot save content elements with max items set to 1 #36

Closed andreashager closed 6 years ago

andreashager commented 6 years ago

Hallo,

there is a problem when I set the maxitems to 1 in the backend layout. Then the only content element for this colPos cannot be saved anymore and the following message is shown:

Sorry, you didn't have proper permissions to perform this change.

Maximum number of allowed content elements (1) reached. 1494605357

The problem only occurs if the maxitems is set to 1, when it is set to 2 I can add and save both content elements without any error.

If you need further information pls feel free to ask me 😀

IchHabRecht commented 6 years ago

Hi @andreashager,

Thanks for your report. Unfortunately I'm unable to verify the problem. Actually there is even an existing test for that scenario. Would you mind to share your backend layout configuration and your TYPO3 and content_defender versions?

andreashager commented 6 years ago

@IchHabRecht

Thanks for your reply - sure I can do that 😄

Here are my versions:

TYPO3 8.7.9
content_defender 3.0.1

and here is my backend layout configuration:

mod {
    web_layout {
        BackendLayouts {
            footer {
                title = Footer
                config {
                    backend_layout {
                        rowCount = 2
                        colCount = 1
                        rows {
                            1 {
                                columns {
                                    1 {
                                        name = Content
                                        colPos = 0
                                        maxitems = 1

                                        allowed {
                                            CType = text
                                        }
                                    }
                                }
                            }

                            2 {
                                columns {
                                    1 {
                                        name = Logos
                                        colPos = 1
                                        maxitems = 1

                                        allowed {
                                            CType = image
                                        }
                                    }
                                }
                            }
                        }
                    }
                }
            }
        }
    }
}
andreashager commented 6 years ago

@IchHabRecht

I also did an update now to content_defender 3.0.2 and the problem still occours.

IchHabRecht commented 6 years ago

There wasn't any technical update just compatibility for TYPO3 9.1

andreashager commented 6 years ago

Ah oke I see 😅 😀

IchHabRecht commented 6 years ago

@andreashager With the provided config I'm able to create content records in each column without problems. Have you installed other extensions (gridelements?) as well? What is the exact error your are seeing? What are you exactly doing? 2018-01-31_155637

IchHabRecht commented 6 years ago

Do you have hidden records in the column(s) already? Do you see "Show hidden content elements" at the bottom of the page view? Maybe you can check the checkbox to see hidden elements.

andreashager commented 6 years ago

@IchHabRecht

I had now a look in the database if there are already deleted/hidden entries on this page but there are only 2 entries in tt_content for this pid each with different colPos. So this should be correct 😄

uid: 8, pid: 57, header: test, colPos: 0
uid: 5, pid: 57, header: Logos, colPos: 1

Extensions I installed (I already deactivated all of them to check if the problem still exists, yes it does): "typo3/cms": "8.7.*", "typo3-ter/content-defender": "^3.0", "typo3-ter/dd-googlesitemap": "^2.0", "typo3-ter/ke-search": "^2.0", "typo3-ter/news": "^6.0", "typo3-ter/realurl": "^2.0", "helhum/typo3-console": "^5.0",

What I am exactly doing?

  1. I deleted everything on the page, then empty the recyler.
  2. I try to create a new content element - then I get a screen with the following message: Sorry, you didn't have proper permissions to perform this change. Maximum number of allowed content elements (1) reached. 1494605357 - but the content element got saved 😟
  3. When I try to save this content element again I also get the error message shown above.

grafik

grafik

IchHabRecht commented 6 years ago

Hi @andreashager,

I'm still not able to reproduce the problem. Is there any way I can test it in your system?

andreashager commented 6 years ago

@IchHabRecht Thanks for your help so far, but I have a local working copy so it will be difficult to give you access to the system 😞

https://github.com/IchHabRecht/content_defender/blob/24880df889aaa1074b248024f7123b5097975656/Classes/Form/FormDataProvider/TcaColPosItems.php#L81

In this line the problem occurs: -> !empty($columnConfiguration['maxitems']) -> set to 1 -> $columnConfiguration['maxitems'] -> set to 1 -> $this->contentRepository->countColPosByRecord($record) ->I get a 1 back -> $colPos -> set to 0 -> $result['databaseRow']['colPos'][0] -> set to 0

jdreesen commented 6 years ago

I've got this problem, too.

I'm using:


When saving the record, the colPosCount gets increased by: https://github.com/IchHabRecht/content_defender/blob/85372af37d2dad8e45c9dbaab63108e0a6e1deea/Classes/Hooks/DatamapDataHandlerHook.php#L55 The method returns true, so the if evaluates to false (which means the record is allowed here).

But this method call increases the internal counter: https://github.com/IchHabRecht/content_defender/blob/85372af37d2dad8e45c9dbaab63108e0a6e1deea/Classes/Hooks/AbstractDataHandlerHook.php#L71-L78

Which means that the later condition here evaluates to true, which means the exception is thrown because maxitems is 1 and countColPosByRecord returns 1, too: https://github.com/IchHabRecht/content_defender/blob/85372af37d2dad8e45c9dbaab63108e0a6e1deea/Classes/Form/FormDataProvider/TcaColPosItems.php#L78-L80

The funny thing is, that the content element was already saved like it should!


Maybe the last condition should check the $result['command'], too and only run when it's === 'new' or when !== 'edit'?

I would create a PR, but I'm not that experienced in the TYPO3 internals, so I'm not sure what the right thing to do here would be. But if I change the last if condition to this, the error is gone:

if ($result['command'] === 'new' // or: !== 'edit'
    && !empty($columnConfiguration['maxitems'])
    && $columnConfiguration['maxitems'] <= $this->contentRepository->countColPosByRecord($record)
) {
jdreesen commented 6 years ago

I just gave it a try with maxitems = 2 (without my patch from above) and it works like it should (at first):

There's no error when I create two elements. When I try to create a third one, the exception is thrown like it should be.

But: When I try to edit any one of the two existing elements, the Exception is thrown with:

Sorry, you didn't have proper permissions to perform this change.

Maximum number of allowed content elements (2) reached. 1494605357

which is clearly wrong.

When I apply my patch (adding $result['command'] === 'new' to the if condition) I can edit both of the two existing elements but I'm not able to create a third one.

IchHabRecht commented 6 years ago

Hi @jdreesen,

Are you using any grid elements on the mentioned page? What kind of content type have you created?

IchHabRecht commented 6 years ago

@andreashager Are you using gridelements as well?

jdreesen commented 6 years ago

The page I was testing with has a custom backend layout and the maxitems = 1 restriction is in colPos = 10. This column does not allow any grid elements (other columns allow them, though on my test page all other columns were empty).

The content element I used in the test (which is the only allowed type CType here) is a custom tt_content element which has only a header field and a list of IRRE children (also custom ones).

IchHabRecht commented 6 years ago

Hi @jdreesen,

Would you mind to share your backend layout configuration as well as your content definition? If you want, you can contact me on slack.

jdreesen commented 6 years ago

I just created a PR with the code that fixes the problem for me.

Also, I've sent my backend layout and TCA to @IchHabRecht via slack so she's hopefully able to recreate the problem an verify the fix.

andreashager commented 6 years ago

@IchHabRecht No I don't use grid elements on the complete website and the extension is not installed! 😃

IchHabRecht commented 6 years ago

I'm able to reproduce the issue now and working on a fix.

jdreesen commented 6 years ago

Sounds great, thx :)

IchHabRecht commented 6 years ago

Hi @andreashager @jdreesen,

Would you mind to test branch https://github.com/IchHabRecht/content_defender/tree/fix-maxitems-handling and see if everything works out for you and your configuration.

Thanks in advance!

jdreesen commented 6 years ago

I just tested it with the branch and it works for me now.

Great work, thx! :)

andreashager commented 6 years ago

@IchHabRecht Same for me, I tested it with the branch you provided, works great :) Thanks!

Can I ask what the problem was? :)

IchHabRecht commented 6 years ago

@andreashager

sure :-) Content Defender is called before the record is saved to decide whether an element can be saved or not. Due to the early access the element has no uid yet, just some id starting with NEW. After saving the new record to the database there is no redirect that would enforce a new script start, but the EditController saves and opens the record in one request. So the DataProvider used in FormEngine context was told, there is already one element (the new one) in the column but didn't found out that is the same record due to the missing (new) uid. That's why the exception was thrown then because the column is loaded and cannot store any new record.

The solution is to save the id of the new record inside content_defender and replace it after DataHandler work with the real uid of the record. Then the FormEngine part can find out that the record that should be shown is the one in the column.

andreashager commented 6 years ago

Ah oke I undertand :) Thanks for your help, now everything works! 😀