rss-bridge icon indicating copy to clipboard operation
rss-bridge copied to clipboard
verified publisher icon RSS-Bridge

[PanneauPocketBridge] Use dynamic list of cities

Open floviolleau opened this issue 2 years ago • 6 comments

Hi,

I think I spotted an issue on the AbstractBridge

I added some comment why I was forced to copy/paste a core method or maybe I misunderstood the logic.

Feel free to add comments

floviolleau avatar Dec 18 '23 15:12 floviolleau

Pull request artifacts

Bridge Context Status
PanneauPocket 1 untitled (current) ✔️
PanneauPocket 1 untitled (pr) ⚠️ WARNING Undefined array key 0 at lib/BridgeAbstract.php line 175
⚠️ WARNING foreach() argument must be of type arrayobject, null given at lib/BridgeAbstract.php line 175
⚠️ WARNING Attempt to read property "href" on null at bridges/PanneauPocketBridge.php line 32
⚠️ WARNING Attempt to read property "plaintext" on null at bridges/PanneauPocketBridge.php line 37
⚠️ DEBUG lib/FeedItem.php(118): Expected $uri to be string but got NULL
⚠️ (shutdown) 8192: preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated in bridges/PanneauPocketBridge.php line 38

last change: Monday 2023-12-18 19:53:14

github-actions[bot] avatar Dec 18 '23 15:12 github-actions[bot]

I've just tried it, and it works well for me! Thanks

EVOTk avatar Dec 18 '23 16:12 EVOTk

lint

dvikan avatar Dec 18 '23 19:12 dvikan

thanks for the contribution flovi.

this pr needs improvement.

  1. this pr increases the frontpage size from 1.7M to 2.3M (25%) because of the 13000 options in the list. this not good

  2. the bridge does http call in constructor. not good

  3. use cache to cache the big list of cities

  4. i agree there is a bug in parent method regarding static::PARAMETERS. if you feel brave can you please fix it so we dont have to copy paste the method.

  5. i dont really see a need to do work in the constructor

dvikan avatar Dec 19 '23 04:12 dvikan

Hi,

For item 1, what do you suggest? For item 2, I don't see other solution to initialize the const cities. For item 4, here is the PR https://github.com/RSS-Bridge/rss-bridge/pull/3858

Thanks for the review

floviolleau avatar Dec 26 '23 03:12 floviolleau

sorry dont have mental energy to followup this one

dvikan avatar Mar 31 '24 02:03 dvikan

  1. rss-bridge might need a new feature to support this. a feature which uses ajax to dynamically autocmplete a long list.

we simply cannot render thousands of <option> elements in the html directly.

  1. we could possibly add this PR behind a bridge setting (config.ini.php) which must be opt-in

feel free to make a new PR

dvikan avatar Jun 18 '24 17:06 dvikan