Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-9675

Add option to delete template/theme/imageset when deleting style.

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.0.7-PL1
    • Fix Version/s: 3.0.9-RC1
    • Component/s: Styles
    • Labels:
      None
    • Environment:
      PHP 3.0.7-PL1, MySQL(i) 5.1.24-rc-1

      Description

      When you remove a style via ACP (->Styles, Delete), corresponding row is removed from phpbb_styles, but in three dependent tables (phpbb_styles_template,phpbb_styles_imageset,phpbb_styles_theme) remains rows with a references to a style name and a path to a corresponding data (template/imageset/theme). If another style with the same name is added, this information is not updated either.

      Therefore, if later for some reason you decide to install the style with the same name as before, but uploaded to a different path, it will be installed, but the path (in those 3 dependent tables) will not be updated. When using this 'new' style the board will look to the old location. This can lead to very confusing experience, especially when you used to have, say, a 'my_prosilver' style located in 'styles/prosilver_tmp', and then after some tests and tweaks you decide to make a 'stable' branch, uninstall it clearly from the board, rename dir to something like 'styles/prosilver_my_stable', install the path again... whoops, board error, cannot find template data in 'styles/prosilver_tmp'.

        Issue Links

          Activity

          Hide
          nickvergessen Joas Schilling added a comment -

          The problem here is, that you can use the template, theme and imagesets of a style which must not be installed itself.
          So this templates and more might still be in use under there old name.

          You can't really say for sure, that the "old" template is not used anymore. The only thing I could imagine is, that we check, whether the cfg-file still exists. If so, we can't help it and need to leave it like is and if it does not exist any more, we could just go and update the path.

          Maybe we should print an error in the first case to tell the user, that his still could not be installed correctly, because of an older version being installed, and that he needs to delete them or rename the new-style, if he wants to install a style with the same name.

          Show
          nickvergessen Joas Schilling added a comment - The problem here is, that you can use the template, theme and imagesets of a style which must not be installed itself. So this templates and more might still be in use under there old name. You can't really say for sure, that the "old" template is not used anymore. The only thing I could imagine is, that we check, whether the cfg-file still exists. If so, we can't help it and need to leave it like is and if it does not exist any more, we could just go and update the path. Maybe we should print an error in the first case to tell the user, that his still could not be installed correctly, because of an older version being installed, and that he needs to delete them or rename the new-style, if he wants to install a style with the same name.
          Hide
          narqelion narqelion [X] (Inactive) added a comment -

          You could fix this bug as well as all the others related to the root cause by changing the default behavior to be consistent when installing a style and deleting a style. If you look at the first sentence of this bug report you see the root cause of the issue,

          When you remove a style via ACP (->Styles, Delete), corresponding row is removed from phpbb_styles, but in three dependent tables (phpbb_styles_template,phpbb_styles_imageset,phpbb_styles_theme) remains rows with a references to a style name and a path to a corresponding data (template/imageset/theme).

          That is because when you delete a style the individual style components are not deleted and this is intentional per the ACP language presented during a style delete action:

          Here you can remove the selected style. You cannot remove all the style elements from here. These must be deleted individually via their respective forms.

          Whatever use case used as the basis to that implementation is the problem. When you install a style all components that make up that style are installed. The reverse should happen when you delete (uninstall) the style, you should never be required to manually go in and remove all the style components yourself unless those style components were installed individually (which they can be.) If any component(s) of a style are in use by another installed style the user should be warned during the delete action and they can choose to proceed or abort. Don't just patch another symptom of inconsistent behavior that leads to multiple error conditions for the users, fix the underlying root cause.

          Show
          narqelion narqelion [X] (Inactive) added a comment - You could fix this bug as well as all the others related to the root cause by changing the default behavior to be consistent when installing a style and deleting a style. If you look at the first sentence of this bug report you see the root cause of the issue, When you remove a style via ACP (->Styles, Delete), corresponding row is removed from phpbb_styles, but in three dependent tables (phpbb_styles_template,phpbb_styles_imageset,phpbb_styles_theme) remains rows with a references to a style name and a path to a corresponding data (template/imageset/theme). That is because when you delete a style the individual style components are not deleted and this is intentional per the ACP language presented during a style delete action: Here you can remove the selected style. You cannot remove all the style elements from here. These must be deleted individually via their respective forms. Whatever use case used as the basis to that implementation is the problem. When you install a style all components that make up that style are installed. The reverse should happen when you delete (uninstall) the style, you should never be required to manually go in and remove all the style components yourself unless those style components were installed individually (which they can be.) If any component(s) of a style are in use by another installed style the user should be warned during the delete action and they can choose to proceed or abort. Don't just patch another symptom of inconsistent behavior that leads to multiple error conditions for the users, fix the underlying root cause.
          Hide
          nickvergessen Joas Schilling added a comment -

          I just added 3 more fields to the delete screen to allow deleting the components while deleting a full-style.

          Show
          nickvergessen Joas Schilling added a comment - I just added 3 more fields to the delete screen to allow deleting the components while deleting a full-style.
          Hide
          narqelion narqelion [X] (Inactive) added a comment -

          I just added 3 more fields to the delete screen to allow deleting the components while deleting a full-style.

          Don't forget to update the language variable in ../language/en/acp/styles.php to reflect that you can now delete the style elements

          'DELETE_STYLE_EXPLAIN' => 'Here you can remove the selected style. You cannot remove all the style elements from here. These must be deleted individually via their respective forms. Take care in deleting styles there is no undo facility.',

          Show
          narqelion narqelion [X] (Inactive) added a comment - I just added 3 more fields to the delete screen to allow deleting the components while deleting a full-style. Don't forget to update the language variable in ../language/en/acp/styles.php to reflect that you can now delete the style elements 'DELETE_STYLE_EXPLAIN' => 'Here you can remove the selected style. You cannot remove all the style elements from here. These must be deleted individually via their respective forms. Take care in deleting styles there is no undo facility.',
          Hide
          nickvergessen Joas Schilling added a comment -

          Fixed.

          Show
          nickvergessen Joas Schilling added a comment - Fixed.
          Hide
          bantu Andreas Fischer added a comment -

          If the code in the patch is copied from somewhere else, functions should be used instead of duplicating the code.

          Show
          bantu Andreas Fischer added a comment - If the code in the patch is copied from somewhere else, functions should be used instead of duplicating the code.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          It is still possible to create a broken style by deleting a style without deleting its elements and subsequently installing a style with the same name from a different directory. There is not even a warning that old elements are going to be used. I will file a new ticket for that.

          Regardless, the option to delete elements implemented in this ticket is useful.

          Show
          Oleg Oleg [X] (Inactive) added a comment - It is still possible to create a broken style by deleting a style without deleting its elements and subsequently installing a style with the same name from a different directory. There is not even a warning that old elements are going to be used. I will file a new ticket for that. Regardless, the option to delete elements implemented in this ticket is useful.
          Hide
          Oleg Oleg [X] (Inactive) added a comment -

          I only tested the basics - not inheritance and not usage of elements since I have no experience with either.

          Show
          Oleg Oleg [X] (Inactive) added a comment - I only tested the basics - not inheritance and not usage of elements since I have no experience with either.

            People

            • Assignee:
              nickvergessen Joas Schilling
              Reporter:
              BioLogIn BioLogIn
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development