Bug #56362

typo3conf/PackageStates.php constantly changes

Added by Christian Weiske over 5 years ago. Updated 12 months ago.

Status:
Closed
Priority:
Should have
Assignee:
Category:
Extension Manager
Target version:
-
Start date:
2014-02-27
Due date:
% Done:

30%

TYPO3 Version:
6.2
PHP Version:
5.4
Tags:
Complexity:
Is Regression:
No
Sprint Focus:

Description

When cleaning the cache (e.g. by manually removing typo3temp/Cache/*) the file typo3conf/PackageStates.php gets regenerated.

The regenerated file has the same contents as before, but in a different order.

This leads to the problem that git sees the file as modified, and we either have to revert it or commit the new version.

Please please please keep the same order when regenerating PackageStates.php, or order the keys by name.


Related issues

Duplicated by TYPO3 Core - Feature #57685: Sort PackageStates.php by extension key Closed 2014-04-06
Duplicated by TYPO3 Core - Bug #60336: Sort package states before serializing them Closed 2014-07-15

Associated revisions

Revision 8eca965b (diff)
Added by Markus Klein about 5 years ago

[TASK] Keep the changes to PackageStates.php low

Introduce a sort operation of the packages before determining
the loading order.
This way the number of changes made to the PackageStates.php file
are reduced.

This is only a first step, further improvements have to be made
to finally resolve #56362.

Resolves: #56362
Releases: 6.3, 6.2
Change-Id: Id6cc4cc2d3f4d9f00e90f355bdede1afe20f57af
Reviewed-on: http://review.typo3.org/27932
Reviewed-by: Stefan Neufeind <>
Reviewed-by: Christian Kuhn <>
Tested-by: Christian Kuhn <>
Reviewed-by: Susanne Moog <>
Tested-by: Susanne Moog <>
Reviewed-by: Anja Leichsenring <>
Tested-by: Anja Leichsenring <>

Revision 60ae97a5 (diff)
Added by Markus Klein about 5 years ago

[TASK] Keep the changes to PackageStates.php low

Introduce a sort operation of the packages before determining
the loading order.
This way the number of changes made to the PackageStates.php file
are reduced.

This is only a first step, further improvements have to be made
to finally resolve #56362.

Resolves: #56362
Releases: 6.3, 6.2
Change-Id: Id6cc4cc2d3f4d9f00e90f355bdede1afe20f57af
Reviewed-on: http://review.typo3.org/31774
Reviewed-by: Anja Leichsenring <>
Tested-by: Anja Leichsenring <>

History

#1 Updated by Christian Weiske over 5 years ago

Happens with TYPO3 6.2beta5.

#2 Updated by Markus Klein over 5 years ago

This is somehow weird. The sorting of the entries is generated by a tree traversing algorithm. As long as the graph does not change, this should always give you the same results.
So we might need to figure out, why the graph changes. (I suspect something like the ordering of the packages discovered in the file system is changing)

#3 Updated by Thomas Maroschik over 5 years ago

Does it happen on current master? I refactored the dependency algorithm after the beta.

#4 Updated by Christian Weiske over 5 years ago

I'm trying with beta5 only, but I will try again with master.

And it really only happens when extensions are activated/deactivated as Markus pointed out - but the problem remains that the diff is huge, when it should only remove/add a single array entry.

So, I'll try with master now.

#5 Updated by Christian Weiske over 5 years ago

I tried with 7389b684 (11 commits ago) because 6e9e545 broke nearly everything here.

The problem still persists.

When activating a single extension, PackageStates.php is completely reordered.
Deactivating that extension again moves the extension's array up to a different place, while the rest is as before:

diff --git typo3conf/PackageStates.php typo3conf/PackageStates.php
index e8c0193..559c2bb 100644
--- typo3conf/PackageStates.php
+++ typo3conf/PackageStates.php
@@ -462,6 +462,12 @@
       'packagePath' => 'typo3conf/ext/aida_fal_dam/',
       'classesPath' => 'Classes/',
     ),
+    'nr_testing' => 
+    array (
+      'state' => 'inactive',
+      'packagePath' => 'typo3conf/ext/nr_testing/',
+      'classesPath' => 'Classes/',
+    ),
     'linkvalidator' => 
     array (
       'manifestPath' => '',
@@ -556,12 +562,6 @@
       'packagePath' => 'typo3conf/ext/devlog/',
       'classesPath' => 'Classes/',
     ),
-    'nr_testing' => 
-    array (
-      'state' => 'inactive',
-      'packagePath' => 'typo3conf/ext/nr_testing/',
-      'classesPath' => 'Classes/',
-    ),
     'aida_unittest' => 
     array (
       'state' => 'inactive',

Why can't it stay at the same place?

#6 Updated by Markus Klein over 5 years ago

Tom, any idea if we can do anything about this?
Only point to tackle this would be to sort all packages by name on each level in the tree first, before traversing it.

#7 Updated by Thomas Maroschik over 5 years ago

I don't think so. Potentially we have to rethink our algorithm again.
But sorting the packages by name before sorting it by dependency could help.

#8 Updated by Markus Klein over 5 years ago

To my best knowledge I don't know any algorithm that does a better job here.

Example:

cms
 |- myext
     |- dependingext
 |- myotherext

This is equivalent in terms of loading order to:

cms
 |- myotherext
 |- myext
     |- dependingext

I've an idea. I'll push a patch here. Please test if thats works, Christian.

#9 Updated by Gerrit Code Review over 5 years ago

  • Status changed from New to Under Review

Patch set 1 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/27932

#10 Updated by Gerrit Code Review over 5 years ago

Patch set 2 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at https://review.typo3.org/27932

#11 Updated by Florian Seirer over 5 years ago

I'm on 6.2.0, and this leads to extensions not working anymore.

In my case I use lonewsdownloads, which adds a description field no tt_news records.
(BTW, I was able to patch the extension manually for 6.2 compatibility.)

But the description field is only added if the configuration of lonewsdownloads is written at the end of PackageStates.php and clear the system cache.
So everytime I (un)install extension I have to rewrite PackageStates.php myself.

#12 Updated by Stefan Neufeind over 5 years ago

Imho thats the default of EXT:lonewsdownloads. It builds upon tt_news? But TER says it does not have a dependency on tt_news. Please try adding a dep in ext_emconf.php and see if that gives you a correct order.

#13 Updated by Markus Klein over 5 years ago

I agree with Stefan. This is NOT a core bug, but a missing dependency in the EXT:lonewsdownloads.

#14 Updated by Florian Seirer over 5 years ago

Yes, that did it. Thank you.

And sorry for interrupting, but hopefully someone else will find this information useful.

#15 Updated by Markus Klein over 5 years ago

Florian, please inform the extension author or file a report in the respective bug tracker.

#16 Updated by Florian Seirer over 5 years ago

The bug tracker for lonewsdownloads doesn't work (see http://forge.typo3.org/projects/extension-lonewsdownloads/issues/new ), so I wrote her an email.
So far, no response.

Who else can fix the forge configuration? And sorry for hijacking this issue.

#17 Updated by Markus Klein over 5 years ago

I'll forward this to the admin team. But don't expect too much, maybe the author does not want to have a tracker. ;-)

#18 Updated by Markus Klein over 5 years ago

Confirmed by admin team: The project owner has to update the project settings. It's missing the trackers to use.

#19 Updated by Markus Klein over 5 years ago

  • Status changed from Under Review to Accepted

Abandoned the first solution as it does not work.

#20 Updated by Lars Trebing about 5 years ago

The first solution may not completely solve Christian’s problem, but at least it leads to deterministic behavior (and much smaller diffs) which I would view as a huge improvement.

#21 Updated by Markus Klein about 5 years ago

deterministic behaviour

Well IMHO that is not quite correct as the behaviour is always deterministic... we do not have any randomness here.

Still, if you say the diffs are better for you we can consider merging this as a first step, yes.

#22 Updated by Gerrit Code Review about 5 years ago

  • Status changed from Accepted to Under Review

Patch set 3 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/27932

#23 Updated by Gerrit Code Review about 5 years ago

Patch set 4 for branch master of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/27932

#24 Updated by Gerrit Code Review about 5 years ago

Patch set 1 for branch TYPO3_6-2 of project Packages/TYPO3.CMS has been pushed to the review server.
It is available at http://review.typo3.org/31774

#25 Updated by Markus Klein about 5 years ago

  • Status changed from Under Review to Resolved
  • % Done changed from 0 to 100

#26 Updated by Markus Klein about 5 years ago

  • Status changed from Resolved to Accepted

First partial fix was merged.
Reopening the ticket here to wait for a final solution.

#27 Updated by Markus Klein about 5 years ago

  • % Done changed from 100 to 30

#28 Updated by Lars Trebing about 5 years ago

By the way, this should somehow be marked as a breaking change, because it will sometimes result in crashes when extensions like lonewsdownloads don’t properly declare their dependencies (cf. <https://forge.typo3.org/issues/56362#note-11&gt;. While this is clearly the respective extension author’s fault, I am sure that it will break a noticeable number of setups that have just been lucky so far.

#29 Updated by Christian Kuhn about 5 years ago

If an extension depends on another one without stating this dependency correctly, this is clearly a bug in the extension and the core can to do much about that. Also, I have no good idea on how to "mark that as breaking" with the current 6.2 thing. We established a system to hint people for possible breaking changes for the 7.0 release by documenting them along the patch in ReST within the core, but that is currently on-hold since the according blueprint was stopped.

#30 Updated by Markus Klein about 5 years ago

@Lars: The comment you refer to is not related to this ticket at all. The changing sorting order will also happen after this particular problem has been fixed. Moreover manual changes to the PackageStates file are not expected and will be overwritten without prior notice.
Simply define a dependency for the extension and you will not have any problems, no matter if the sorting of the PackageStates file changes (which this ticket is about).
Btw. We're not talking about a changing of the sorting order that affects the loading order of extensions. We only try to improve the stability of the sorting order of extensions that do NOT depend to each other such that their loading order does not matter.

#31 Updated by Lars Trebing about 5 years ago

We’ll certainly all agree that this is the extension author’s fault and none of the core’s responsibility, and I was 100% in favor of making this change (and still am 100% in favor of keeping it). It’s just that all those non-developer integrators out there are already struggling with (and complaining about) how the cleanup/modernization/refactoring/etc. work that’s been done in the past few years keeps breaking their beloved extensions and whining about how much better everything was in the good old times. So just in order to avoid giving them another thing to despair and whine about, I think it would be nice if we could maybe just add a little helpful hint somewhere in the release notes or so, where they can easily find it and propagate it into their favorite blogs and forums.

#32 Updated by Markus Klein about 5 years ago

Lars, what exactly do you want to write into the release notes?
I just don't get your point here, sorry.

#33 Updated by Christian Toffolo about 5 years ago

I'm using 6.2.5-dev that has 8eca965bff4a9f917c9c59e970bdc9269d3e1503 and the number of changes in PackageStates.php is still huge.

#34 Updated by Markus Klein about 5 years ago

@Ian: Yes I'm aware of that, but still I didn't have the time to do a big debugging session to improve this. It's not so nice to read adjacent matrices from PHP arrays, you know. But that's what is necessary to figure out which graphs are generated and why those graphs tend to change so vastly.

#35 Updated by Christian Toffolo about 5 years ago

Can't PackageStates.php be sorted alphabetically after generation?
(Sorry if it's a stupid question, I'm not an expert)

#36 Updated by Stefan Neufeind about 5 years ago

It takes required loading-order/dependencies into account.

#37 Updated by Markus Klein about 5 years ago

I rephrase the answer of Stefan slightly: The sorting of the extensions defines the loading order, so the sorting is crucial.

Alphabetical sorting is only possible for extensions that are on the "same loading order level", such that their exact order with each other does not matter.

#38 Updated by Christian Toffolo almost 5 years ago

What if another element 'loadOrder' is added to the array returned by PackageStates.php?
This element 'loadOrder' has simply a list of the extensions names in order of loading while 'packages' remain the same as of now but the extensions are in alphabetical order.

#39 Updated by Markus Klein almost 5 years ago

Hi Ian!
This does not solve the problem.
The diffs will be smaller, but the ordering within the list would still change vastly.

#40 Updated by Christian Toffolo almost 5 years ago

Hi! I understand that it doesn't solve the problem completely but it's much easier to track diffs of a simple list of keys ('loadOrder') than in a list of keys ('packages') that has sub-elements ('manifestPath'; 'packagePath'; 'classesPath'; etc.) cause with a so complex structure diff tools get crazy ;)

If the solution that I mentioned is feasible then 'packages' sub-array will change only if a extensions are added/removed to/from the installation while the 'loadOrder' key will have the big changes and with 8eca965bff4a9f917c9c59e970bdc9269d3e1503 these changes will be very small.

#41 Updated by Markus Klein almost 5 years ago

Well, I generally ask myself why you guys worry so much about this internal PackageStates file. I never had to touch its contents.
It's ok to have it in a repository as backup, but why are you actually interested in its content?

You're welcome to push a patch that introduces this additional list of course.

#42 Updated by Christian Toffolo almost 5 years ago

The answer of your question is in the bug report and indeed patches are welcome.

#43 Updated by Markus Klein almost 5 years ago

Actually there's no answer to my question. Even if the file changes on a cache clear, you don't even need to backup the file, since the system behavior does not change.

#44 Updated by Stefan Neufeind almost 5 years ago

Well, it contains the list of active/inactive extensions. So it's also part of the "configuration" you will want to backup, check into a local git or maybe transport from dev from prod during a rollout.

#45 Updated by Markus Klein almost 5 years ago

Yes, of course you will backup the file if you add/remove extensions, but why in between?
Still you're not interested into the file's content, are you?

Don't get me wrong, we should of course keep the diff as small as possible, but I currently see this as quite some work and I doubt this work really saves that much time for the few people actually looking into the diff of the PackageStates which would justify the necessary effort.

#46 Updated by Jonathan Starck almost 5 years ago

Maybe one reason to edit the PackageStates.php is as Example caretaker.
You only able to use a TYPO3 Instance as Caretaker Server when you install first caretaker an then caretaker_instance. After you add a new Extension or something like this and the order is changed - the functionality of caretaker breaks.

#47 Updated by Markus Klein almost 5 years ago

That is a bug in caretaker then; missing dependency from caretaker_instance on caretaker?

#48 Updated by Jonathan Starck almost 5 years ago

With Caretaker you watch your caretaker instance.
You only need at the TYPO3 both Extensions when it is the server of Caretaker. On the Instance you want to watch you only need caretaker instance.

But nice to know - so I can fix my issue by hard coding ^^

#49 Updated by Markus Klein almost 5 years ago

Ah, I see. That's another example for the missing "before" and "after" dependencies.

#50 Updated by Stefan Busemann almost 5 years ago

A before and after dependency is really necessary. We have to set the PackageStates.php to readonly, that it works in our large projects. Besides that, it is also a solution missing, to conditional activate or deactivate packages / extensions. In the past this was very easy, by extending the localconf.php by another php file and defining there another extlist or search and replace extension keys.

#51 Updated by most wanted almost 5 years ago

Stefan Busemann wrote:

A before and after dependency is really necessary. We have to set the PackageStates.php to readonly, that it works in our large projects. Besides that, it is also a solution missing, to conditional activate or deactivate packages / extensions. In the past this was very easy, by extending the localconf.php by another php file and defining there another extlist or search and replace extension keys.

http://typo3blogger.de/how-to-install-extensions-only-in-local-development-environment-in-typo3-6-2/

#52 Updated by Nicole Cordes over 4 years ago

  • Status changed from Accepted to Needs Feedback
  • Assignee set to Nicole Cordes

I did some testing with TYPO3 7 (current master) as well as with 6.2 (current branch) and can't find any changes after deleting caches. Can you please verify that this issue can be marked as resolved?!

#53 Updated by Christian Weiske over 4 years ago

The patch made it better than it was before, so I'd see the current issue as resolved.

There may be ways to make it even better (e.g. to not take the "active" state into account when sorting the array), but someone else can come up with a patch for that in another issue.

#54 Updated by Nicole Cordes over 4 years ago

  • Status changed from Needs Feedback to Resolved

As the PackageStates.php would change with activating / deactivating an extension, I don't see the reason to change the sorting ;-)

#55 Updated by Benni Mack 12 months ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF